-
Notifications
You must be signed in to change notification settings - Fork 1k
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
feat: Arrays should be indexable from the end too #3004
Conversation
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.
Thanks @ouertani this is looking good.
Would you mind adding a new array.json
test file for QueryTranslationTest
to cover existing + new functionality around arrays please?
ksql-engine/src/main/java/io/confluent/ksql/codegen/SqlToJavaVisitor.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.
LGTM, thanks @ouertani
(Note, I've made some small corrections to indentation and added test cases to cover out-of-bounds behaviour).
One final thing before we merge this... is there any documentation that needs updating to cover this new functionality?
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.
LGTM! I think your suggestion is the right place for the docs - we can merge as soon as we reflect that there.
ksql-engine/src/test/java/io/confluent/ksql/codegen/SqlToJavaVisitorTest.java
Outdated
Show resolved
Hide resolved
Hey @ouertani, just waiting on the docs to merge this! |
@big-andy-coates documentations updated. |
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.
LGTM, with a couple of suggestions.
Co-Authored-By: Jim Galasyn <[email protected]>
Co-Authored-By: Jim Galasyn <[email protected]>
Description
Fixes #2974 Arrays should be indexable from the end too
Testing done
Test case add 3872065#diff-7cd71dc0c9567e04c842d2682acc9b5eR110
Manual test as well has been done
Reviewer checklist