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

Add value traversal methods to Type #12797

Merged
merged 4 commits into from
Nov 18, 2022

Conversation

findepi
Copy link
Member

@findepi findepi commented Jun 10, 2022

#12795 introduces some awesome value traversal utilities in Iceberg connector.
This PR makes them available to everyone.

@cla-bot cla-bot bot added the cla-signed label Jun 10, 2022
@findepi
Copy link
Member Author

findepi commented Jun 10, 2022

Relates to #3316

@findepi
Copy link
Member Author

findepi commented Jun 22, 2022

@martint @losipiuk @sopel39 @rzeyde-varada @raunaqmorarka
what do you think about general direction?
Let's have agreement before i spend more time on this.

If we don't want to add this to Type, we just can keep having the value traversal code within Iceberg plugin.

@losipiuk
Copy link
Member

I like the interface. The other option to model that would be a new operator. But then it would be hard to consume that in the Iceberg connector - which is the sole use for now.

@findepi
Copy link
Member Author

findepi commented Jun 22, 2022

thanks @losipiuk

@martint @sopel39 @rzeyde-varada @raunaqmorarka thoughts?

@raunaqmorarka
Copy link
Member

thanks @losipiuk

@martint @sopel39 @rzeyde-varada @raunaqmorarka thoughts?

The new API lgtm, we should be able to use it in #3316 to find adjacent value and then use existing methods for equality check with adjacent value for implementing areConsecutive

@findepi
Copy link
Member Author

findepi commented Jul 21, 2022

@martint thoughts?

@findepi
Copy link
Member Author

findepi commented Sep 17, 2022

apparently not useful, hence maybe no thoughts.

@findepi findepi closed this Sep 17, 2022
@findepi findepi deleted the findepi/type-value-traversal branch September 17, 2022 16:03
@findepi findepi restored the findepi/type-value-traversal branch October 18, 2022 08:00
@findepi
Copy link
Member Author

findepi commented Oct 18, 2022

Needed for #14452

@findepi findepi reopened this Oct 18, 2022
@findepi findepi force-pushed the findepi/type-value-traversal branch 3 times, most recently from 3de0faf to eade8ec Compare October 19, 2022 12:21
@findepi findepi marked this pull request as ready for review October 19, 2022 12:22
@findepi findepi force-pushed the findepi/type-value-traversal branch from eade8ec to 37956ed Compare October 20, 2022 14:21
@findepi findepi mentioned this pull request Oct 20, 2022
@findepi findepi force-pushed the findepi/type-value-traversal branch from 37956ed to 3175ae7 Compare October 25, 2022 10:32
@findepi findepi requested a review from losipiuk November 4, 2022 15:25
Copy link
Member

@losipiuk losipiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I liked it before. Stil like it.

@findepi findepi force-pushed the findepi/type-value-traversal branch from 3175ae7 to cfdd3cd Compare November 9, 2022 14:14
@findepi
Copy link
Member Author

findepi commented Nov 9, 2022

(just rebased on current state of #14747)

/**
* Returns the maximum value that compares less than {@code value}.
* <p>
* The type of the value must match {@link #getJavaType}.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can't match getJavaType. For types such as smallint, integer, bigint, getJavaType returns long.class. This method takes an Object, so it doesn't support primitive values (they would need to be boxed).

In general, I'm not sure I like how this method tries to be generic for all types but:

  • It's only supported for orderable types
  • The signature is not tight enough to represent all carrier types safely.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can't match getJavaType. For types such as smallint, integer, bigint, getJavaType returns long.class. This method takes an Object, so it doesn't support primitive values (they would need to be boxed).

yes, I meant boxing here

btw this is modeled like we did for getRange in e23e317#diff-3eedf5cda66ac7a68eb48f4eee146a4ccf5658fd07436ba5b279a089294a5c0cR174

It's only supported for orderable types

yes, same as getRange
non-orderable types will remove Optional.empty() since there is no "next" nor "previous"

The signature is not tight enough to represent all carrier types safely.

this part i did not understand.
@martint can you please clarify?

@findepi
Copy link
Member Author

findepi commented Nov 16, 2022

Added value traversal for few more types, so that Iceberg-side of things is fully redundant.
While Iceberg is the only use-place for now, this gonna be used in #14452 too.

@findepi
Copy link
Member Author

findepi commented Nov 16, 2022

@losipiuk @martint PTAL

@findepi
Copy link
Member Author

findepi commented Nov 16, 2022

CI #14441

@findepi findepi self-assigned this Nov 18, 2022
@findepi findepi added the no-release-notes This pull request does not require release notes entry label Nov 18, 2022
@findepi findepi merged commit be73377 into trinodb:master Nov 18, 2022
@findepi findepi deleted the findepi/type-value-traversal branch November 18, 2022 21:19
@hashhar hashhar added this to the 404 milestone Nov 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed no-release-notes This pull request does not require release notes entry
Development

Successfully merging this pull request may close these issues.

5 participants