Accumulo type mapping cleanup #3559
Conversation
|
Cool! +1 |
The `Field`'s `toString` is complex, as if it was playing some conversion role, but it does not. Simplify it and make more robust (avoid throwing exceptions), at the cost of not producing human-readable string representation of collections.
|
For some context here, most of this code was for downstream applications that use |
|
@adamjshook thanks for looking into this. OTOH, i tried to take a quick stab at the #3558, and i found this flexibility limiting. BTW i added some more changes to the PR. Please take another look. |
The conversion code was flexible, allowing different representations for given Presto Type. This is unnecessary, as the value passed is always a Presto Type's stack representation.
|
thanks @adamjshook for the review |
|
@adamjshook The connector documentation mentions using the connector JAR as a server-side iterator: https://prestosql.io/docs/current/connector/accumulo.html#installing-the-iterator-dependency Is this still used? Are there tests for it? Will this PR break it? It sounds like this is something we'll need to address when moving Presto to Java 11. Maybe by pulling that out into a separate module that builds with Java 8. |
|
@electrum Hi David -- Yes, it is still required in the connector here. I do not see any tests in this repository for the Accumulo iterators, however I do not believe this PR will cause any issues with the iterator. If how the data was stored in Accumulo was being modified, then it would likely break the iterator code. I think a separate module for Java 8 would be the way to go for the iterator dependency. Java 11 support was recently added for Accumulo (although the Presto integration hasn't been tested AFAIK), but older Accumulo versions would still require a Java 8 version. |
|
What's the entry point in the module for the accumulo iterators? |
|
The servers need these two classes on their classpath in order for the connector to work properly: https://github.com/prestosql/presto/tree/master/presto-accumulo/src/main/java/io/prestosql/plugin/accumulo/iterators |
| if (!(value instanceof Block)) { | ||
| throw new PrestoException(FUNCTION_IMPLEMENTATION_ERROR, "Object is not a Block, but " + value.getClass()); | ||
| } | ||
| // Block |
There was a problem hiding this comment.
Should we enforce the types here?
return (Block) value;There was a problem hiding this comment.
I'd like to do this, but intellij frowns on "unnecessary cast". (which i dont want to ignore in general, although here it's incorrect).
This is, however, enforced by the checkArgument(Primitives.wrap(type.getJavaType()).isInstance(value), check above.
|
@adamjshook Thanks for the explanation about the iterators. I only see two iterators, Are the ones in |
|
@electrum Yes, those were experimental at the time and are no longer needed. There are no plans to put these into the Accumulo distribution, and I am not entirely sure they would be worth while. They are pretty coupled for Presto and are not very general-purpose. With that said, I am looking into breaking the iterators out into a separate module. Feel free to open an issue and assign it to me if you'd like to track it better. |
Removes some unnecessary flexibility in the code, making it easier to do
things like #3558 in the future.