[BEAM-9379] Simplify BeamCalcRel inputs #13930
Conversation
apilloud
changed the title
apilloud
changed the title
| value = Expressions.call(expression, "getArray", Expressions.constant(index)); | ||
| if (storageType == Object.class | ||
| && TypeName.ROW.equals(fieldType.getCollectionElementType().getTypeName())) { | ||
| // Workaround for missing row output support |
There was a problem hiding this comment.
This is the one special case that remains, it requires rewriting the output code to support rows. That is the next PR.
|
R: @ibzib |
ibzib
left a comment
ibzib
left a comment
There was a problem hiding this comment.
LGTM. But I think this would be a good time to add some comments and rename some methods to make this class easier to follow.
| if (value.getType() == java.sql.Time.class) { | ||
| valueDateTime = Expressions.call(BuiltInMethod.TIME_TO_INT.method, valueDateTime); | ||
| } else if (value.getType() == Long.class) { | ||
| } else if (value.getType() == Integer.class || value.getType() == Long.class) { |
There was a problem hiding this comment.
Why does this need to include Integer now?
There was a problem hiding this comment.
The internal type is actually suppose to be int. We've been passing Calcite a Long instead and the compiler just happily upconverts when doing math between the two types. We are still doing math somewhere (window functions?) that turns this into a Long sometimes. I think even before this change we were at risk of receiving a Integer here. (In the next CL I'm switching this to the java Number interface so we are permissive on outputs.)
https://github.com/apache/calcite-avatica/blob/89e0deb510311b85b8c8bacde6d2ff70c309930e/core/src/main/java/org/apache/calcite/avatica/SqlType.java#L306
...va/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/rel/BeamCalcRel.java
Show resolved
Hide resolved
...va/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/rel/BeamCalcRel.java
Outdated
Show resolved
Hide resolved
| private static Expression value(Expression value, FieldType fieldType) { | ||
| switch (fieldType.getTypeName()) { | ||
| case BYTE: | ||
| return Expressions.convert_(value, Byte.class); |
There was a problem hiding this comment.
Why are these conversions necessary? e.g. doesn't getByte already return Byte?
There was a problem hiding this comment.
These may be coming out of a Map or List as an Object. This isn't strictly necessary, I'm doing it to ensure types are what we expect. It generates a cast which results in a ClassCastException if it isn't the type we expected.
...va/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/rel/BeamCalcRel.java
Outdated
Show resolved
Hide resolved
2 participants
This is removing logical type optimizations broken by #11074 and removes the passthrough Beam types as
Objectoptimization. After this change, inputs are always converted to Calcite types except for one special case of Row (which will be fixed in the next PR).Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.