-
Notifications
You must be signed in to change notification settings - Fork 4.5k
[BEAM-9379] Simplify BeamCalcRel output conversion #14518
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
kennknowles
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this have a performance cost?
|
I ran our nexmark tests and the result was within the noise bounds of our performance dashboard. I don't think there is a huge benefit to code generation for this piece. What code generation gives us here is unrolled loops and dead-code elimination, both of those will be done by the Java runtime. |
|
This one tries to be very generous on the types it accepts from Calcite. I added tests that covered a few cases that didn't appear to have coverage. |
ibzib
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is much easier to read than the generated code before.
I'm generally unsure about why we can expect such a wide range of Java types for each schema type. Do we have an idea about where the variance comes from (Calcite?)?
...a/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/BeamSqlDslUdfUdafTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, maybe I can reuse these for ZetaSQL eventually.
...a/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/BeamSqlDslUdfUdafTest.java
Outdated
Show resolved
Hide resolved
...a/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/BeamSqlDslUdfUdafTest.java
Outdated
Show resolved
Hide resolved
...a/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/BeamSqlDslUdfUdafTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know why this happens? Is there a bug tracking it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened https://issues.apache.org/jira/browse/BEAM-12175, I'm not sure for this one but it may be related to the issue with Numbers.
...a/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/BeamSqlDslUdfUdafTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any idea why there are so many different possible types here? Same question for everywhere else we can't just do a simple cast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the tests that fail, Calcite will probably consider this a feature. It looks like when the implementation for an internal operator is missing, Calcite just substitutes in a 'compatible' one. For example Integer round(Integer x) can silently become Long round(Long x). I think there are possibly some cast operations that are being treated as no-op as well. This bug is preexisting, but I opened https://issues.apache.org/jira/browse/BEAM-12176
This change makes BeamCalcRel output a Calcite row, then converts to a Beam row using normal code (rather than generated). This makes things much easier to debug and fixes nested rows.
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.