Skip to content
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

Merged
merged 11 commits into from
Jul 11, 2019

Conversation

ouertani
Copy link
Contributor

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

ksql> SELECT interests[-1] FROM users_extended Limit 1;
News
Limit Reached
Query terminated
ksql> SELECT interests[-1], interests FROM users_extended Limit 1;
News | [Game, News]
Limit Reached
Query terminated
ksql> SELECT interests[-10], interests FROM users_extended Limit 1;
null | [Game, News]
Limit Reached
Query terminated
ksql> SELECT interests[1], interests FROM users_extended Limit 1;
News | [Game, News]
Limit Reached
Query terminated
ksql> exit

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • [ X] Ensure relevant issues are linked (description should include text like "Fixes #")

@ouertani ouertani requested a review from a team as a code owner June 21, 2019 22:22
@big-andy-coates big-andy-coates changed the title Fixes #2974 Arrays should be indexable from the end too feat: Arrays should be indexable from the end too Jun 27, 2019
Copy link
Contributor

@big-andy-coates big-andy-coates left a 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?

@ouertani
Copy link
Contributor Author

Copy link
Contributor

@big-andy-coates big-andy-coates left a 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?

@big-andy-coates big-andy-coates requested a review from a team June 28, 2019 18:43
@ouertani
Copy link
Contributor Author

Copy link
Contributor

@agavra agavra left a 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.

@big-andy-coates
Copy link
Contributor

Hey @ouertani, just waiting on the docs to merge this!

@ouertani ouertani requested a review from JimGalasyn as a code owner July 3, 2019 16:13
@ouertani
Copy link
Contributor Author

ouertani commented Jul 3, 2019

@big-andy-coates documentations updated.

Copy link
Member

@JimGalasyn JimGalasyn left a 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.

ouertani and others added 2 commits July 4, 2019 20:53
Co-Authored-By: Jim Galasyn <jim.galasyn@confluent.io>
Co-Authored-By: Jim Galasyn <jim.galasyn@confluent.io>
@big-andy-coates big-andy-coates merged commit a166075 into confluentinc:master Jul 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Arrays should be indexable from the end too
4 participants