-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
use default fpp for writing in Hive connector if not provided in table metadata #16589
use default fpp for writing in Hive connector if not provided in table metadata #16589
Conversation
@Praveen2112 Hey, I was not sure who to tag so I tagged you as a reviewer because I noticed that you made changes to orc bloom filter previously. I hope you don't mind :) |
fa34f4f
to
3b1b292
Compare
Hi ✋ PTAL whenever you have time. Thx :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me but can we add some test coverage here ? But even the table properties are default - doesn't hive specify them when creating a table ?
I will add it soon.
Yes, it does not specify when creating a table. when I do show create tables after creating in hive, it does not add When you look up
So one can wonder where does this
|
3b1b292
to
e565c47
Compare
Added test coverage when there isn't fpp value |
e53674e
to
9907887
Compare
PTAL rebased and updated some minor changes :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this issue.
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergFileWriterFactory.java
Outdated
Show resolved
Hide resolved
9907887
to
c88edf0
Compare
607e0f5
to
3ef12a1
Compare
This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua |
@Praveen2112 is this still good to go and we can merge? |
@Chaho12 Thanks for working on this. |
Description
I think that if table metadata does not have FPP value, default fpp value should be used for bloom filter for writing ORC files.
As of now, it returns error saying that
FPP for bloom filter is missing
, which is ironic asAdditional context and related issues
Hive has default bloom filter of 0.05 (5%)
Trino doc saids that default value of fpp is 0.05
Release notes
( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text: