-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Core: Fix overflow due to default value on timestamp nanos #13780
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
Conversation
cde870a to
4c8c522
Compare
|
|
||
| private static Literal<?> defaultFromJson(String defaultField, Type type, JsonNode json) { | ||
| if (json.has(defaultField)) { | ||
| return Expressions.lit(SingleValueParser.fromJson(type, json.get(defaultField))); |
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.
This is a problem for 1.10? Should we block the release on this? While I know the chances are unlikely here but if anyone is trying to set defaults close to Epoch they won't Overflow and will instead write incorrect values
| return Expressions.lit(SingleValueParser.fromJson(type, json.get(defaultField))); | ||
| Object value = SingleValueParser.fromJson(type, json.get(defaultField)); | ||
| if (type instanceof Types.TimestampNanoType) { | ||
| return Expressions.nanos((long) value); |
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.
should we check value not null? the json value itself can also be null, right?
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.
We could precondition this, we will have slightly divergent behavior though. The normal path would bail out here
So as written this would throw an NPE at a different spot only for nanos
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.
yes, this is not an issue. the spec says
Non-null values for initial-default or write-default are invalid.
Earlier, I wasn't sure if null values are allowed for the default attributes.
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
Iceberg throws exceptions if JSON contains default values on timestamp nanos: