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

Remove getters for narrow integral types from LongArrayBlock #19199

Closed

Conversation

raunaqmorarka
Copy link
Member

Description

Originally added as a temporary workaround, but unused now

Additional context and related issues

Related to prestodb/presto#5674

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

@cla-bot cla-bot bot added the cla-signed label Sep 29, 2023
@raunaqmorarka raunaqmorarka requested a review from dain September 29, 2023 03:24
Originally added as a temporary workaround, but unused now
@github-actions github-actions bot added tests:hive hive Hive connector labels Oct 9, 2023
@dain
Copy link
Member

dain commented Oct 17, 2023

Interesting, I have most of this in the #19385 as that code needs code to produce the correct block types.

@raunaqmorarka
Copy link
Member Author

Interesting, I have most of this in the #19385 as that code needs code to produce the correct block types.

I basically got stuck here because io.trino.plugin.hive.coercions.IntegerNumberUpscaleCoercer is actually relying on these methods and I wasn't sure how to avoid that.
Does it make sense to land the rest of the changes to avoid relying on these methods in the other places ?

@dain
Copy link
Member

dain commented Oct 24, 2023

I ended up rewriting a bunch of stuff like that. I have a branch (stuck behind another PR) that remove all of the get methods from the Block interface. With the addition of ValueBlocks and the guarantee that if you can down cast a ValueBlock to the declared type in Type.getValueBlockType, we don't need the interface methods anymore. This is all setup so we can add more types likebyte, short, int, float, and double, and generally simplifies most code (some places got more complex. These changes will let us support all of these types directly in functions without using long everywhere.

Anyway, I'd prefer to wait for my branch these changes will conflict with me.

@dain
Copy link
Member

dain commented Nov 8, 2023

BTW here is my PR that removes all of the getters. #19577

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed hive Hive connector
Development

Successfully merging this pull request may close these issues.

2 participants