Skip to content

Remove duplicate ChunkedSliceOutput class#17034

Merged
findepi merged 2 commits intotrinodb:masterfrom
findepi:findepi/remove-duplicate-chunkedsliceoutput-class-573522
Apr 18, 2023
Merged

Remove duplicate ChunkedSliceOutput class#17034
findepi merged 2 commits intotrinodb:masterfrom
findepi:findepi/remove-duplicate-chunkedsliceoutput-class-573522

Conversation

@findepi
Copy link
Copy Markdown
Member

@findepi findepi commented Apr 14, 2023

No description provided.

@findepi findepi added the no-release-notes This pull request does not require release notes entry label Apr 14, 2023
@cla-bot cla-bot bot added the cla-signed label Apr 14, 2023
@findepi findepi force-pushed the findepi/remove-duplicate-chunkedsliceoutput-class-573522 branch from 7bb0987 to d31d4ea Compare April 14, 2023 12:56
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not delete the one in orc instead ?
Having it here makes it easier for other file formats to use this as well

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

please see where trino-orc is used and where trino-hive-formats is used. do you still think we should do other way?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okay, we shouldn't take dependency of trino-hive-formats in trino-orc as it gets included in iceberg and delta. But now I also think taking dependency of trino-orc in trino-hive-formats is confusing as it looks like trino-hive-formats is supposed to be code for hive file formats other than orc and parquet.
Maybe we move this class to trino-plugin-toolkit ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

awesome idea

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done, ptal

We have two copies of `ChunkedSliceOutputs` and they are almost
identical. The difference looks unintentional, eliminate.
@findepi findepi force-pushed the findepi/remove-duplicate-chunkedsliceoutput-class-573522 branch from d31d4ea to 1a77f0a Compare April 17, 2023 07:03
@findepi
Copy link
Copy Markdown
Member Author

findepi commented Apr 17, 2023

(just rebased to resolve coflicts)

@findepi findepi force-pushed the findepi/remove-duplicate-chunkedsliceoutput-class-573522 branch from 1a77f0a to e511ac4 Compare April 17, 2023 07:08
@findepi findepi requested a review from raunaqmorarka April 17, 2023 07:08
@findepi findepi merged commit d8d40c8 into trinodb:master Apr 18, 2023
@findepi findepi deleted the findepi/remove-duplicate-chunkedsliceoutput-class-573522 branch April 18, 2023 06:24
@github-actions github-actions bot added this to the 414 milestone Apr 18, 2023
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.

3 participants