Iceberg connector support ORC bloom filter feature

[zhengxingmao]

Description

Is this change a fix, improvement, new feature, refactoring, or other?
new feature
Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)
for iceberg connector
How would you describe this change to a non-technical end user or system administrator?
Iceberg connector support ORC bloom filter feature

Related issues, pull requests, and links

Documentation

( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
(x) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
(x) Release notes entries required with the following suggested text:
Iceberg connector support ORC bloom filter feature

# Section
Iceberg connector support ORC bloom filter feature

[cla-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

[raunaqmorarka]

We're missing tests which exercise the functionality.
Please refactor TestOrcWithBloomFilters into a base class (move it to io.trino.testing package) and extend it to test both hive and iceberg connectors.

[zhengxingmao]

At present, the community has not passed the cla protocol submitted by me. After passing, test cases will be added.@raunaqmorarka

[raunaqmorarka]

@martint please help with CLA and enable CI here

[findepi]

I approved the CI run.

@zhengxingmao please rebase and add the tests so that the PR is ready for merge when CLA gets processed.

[cla-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

[zhengxingmao]

I approved the CI run.

@zhengxingmao please rebase and add the tests so that the PR is ready for merge when CLA gets processed.

Done @findepi @raunaqmorarka

[cla-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

[cla-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

[ebyhr]

@cla-bot check

[cla-bot]

The cla-bot has been summoned, and re-checked this pull request!

[alexjo2144]

Can you add some documentation to iceberg.rst?

[findepi]

waiting for @Praveen2112 clarification in #11732 (comment)

@Praveen2112 did you have a chance to look into this?

cc @raunaqmorarka

[Praveen2112]

LGTM. One QQ to @findepi In case of error code can we stick to code from Iceberg instead of Hive ?

If yes can we update the error code ?

[findepi]

pending clarity on #11732 (comment)

[findepi]

@zhengxingmao thanks for removing the sort order.
it's awesome how quickly you adapt your code to review comments

however, we seem to struggle to reach clarity whether/why we wanted the test files to be sorted.
Please wait a bit while @Praveen2112 has time to explain this to me.

[findepi]

@zhengxingmao thanks for your patience. Seems we reached consensus: #11732 (comment)

Can you please add a preparatory commit that removes sorting from hive bloom filter tests?
This should come with a commit message explaining that it was not needed.

I know this adds more work for you, but this way these changes will make more sense, when viewed later on by someone who didn't participate in the discussion here.

[zhengxingmao]

@zhengxingmao thanks for your patience. Seems we reached consensus: #11732 (comment)

Can you please add a preparatory commit that removes sorting from hive bloom filter tests? This should come with a commit message explaining that it was not needed.

I know this adds more work for you, but this way these changes will make more sense, when viewed later on by someone who didn't participate in the discussion here.

Done.Thank you for reviewing.

[findepi]

This is pre-existing, so wasn't a prerequisite for a merge here. @dain @Praveen2112 @raunaqmorarka do you know why default is false?

[raunaqmorarka]

I think there used to be bugs in the past, but not sure why it isn't enabled now. In most cases bloom filter will not exist and it will be a no-op

[findepi]

@raunaqmorarka do you want to switch it on by default now?

[raunaqmorarka]

Yes, AFAIK there shouldn't be a problem in enabling it now

[findepi]

@zhengxingmao thanks for your contribution

[zhengxingmao]

@zhengxingmao thanks for your contribution

@raunaqmorarka @alexjo2144 @Praveen2112 @findepi Thank you for your review.