HIVE-30020: Fix TIMESTAMP WITH LOCAL TIME ZONE data loss and read failure on ORC tables - #6761
vikramahuja1001 wants to merge 3 commits into
Conversation
|
Can you help in reviewing this PR as per your availability? |
| vector.time[rowId] = timestampTZ.toEpochMilli(); | ||
| vector.nanos[rowId] = timestampTZ.getNanos(); | ||
| break; | ||
| } |
There was a problem hiding this comment.
default case is not handled for primitive category due to that the data is getting written in this case. Handle default case and throw exception so that the error will come while writing the data itself for unsupported/unhandled primitive type.
There was a problem hiding this comment.
done, thanks for pointing it out
| .getPrimitiveJavaObject(obj).toSqlTimestamp()); | ||
| break; | ||
| } | ||
| case TIMESTAMPLOCALTZ: { |
There was a problem hiding this comment.
It looks indentation problem for this block, please check and fix.
There was a problem hiding this comment.
I am not able to understand why indentation is being flagged here. Indentation level is same as other case statements above and below. Can you take a look once?
|
|
||
| insert into timestampltz_orc_format select * from timestampltz_formats; | ||
|
|
||
| SELECT * FROM timestampltz_orc_format; No newline at end of file |
There was a problem hiding this comment.
Adding ORDER BY clause to the select query is resulting an exception with the patch, please check.
There was a problem hiding this comment.
Yes rightly pointed, ORDER BY clause is giving an exception. To fix that will require a lot of changes, which are separate from the scope of this PR. I have raised a follow up JIRA and will work on that as well, hope that's fine.
JIRA ID: https://issues.apache.org/jira/browse/HIVE-30042
There was a problem hiding this comment.
I am ok with handling this scenario in a separate JIRA mentioned above. But I request you to add proper details with exception in the description section of the JIRA. Thanks.
There was a problem hiding this comment.
Sure, will do.
23a937d to
cf89476
Compare
|
Thanks for reviewing this PR @mdayakar . I have raised a follow up JIRA to fix the order by query clause as well and will raise a PR for that as well. |
|
@mdayakar , could you please merge this PR as well? |
@vikramahuja1001 I am not a committer to merge this PR. I request @zabetak , @kasakrisz , @soumyakanti3578 to review and merge this PR. |
There was a problem hiding this comment.
🟡 Changes recommended
One or more issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds ORC read/write support for TIMESTAMP WITH LOCAL TIME ZONE and a regression query validating round-trip preservation.
Changes:
- Converts local-time-zone timestamps to ORC timestamp-instant vectors.
- Restores timestamp-zone writable values during ORC reads.
- Adds an ORC round-trip query and expected output.
File summaries
| File | Description |
|---|---|
| ql/src/test/results/clientpositive/llap/timestampz_with_file_formats.q.out | Updated as part of this pull request. |
| ql/src/test/queries/clientpositive/timestampz_with_file_formats.q | Updated as part of this pull request. |
| ql/src/java/org/apache/hadoop/hive/ql/io/orc/WriterImpl.java | Updated as part of this pull request. |
| ql/src/java/org/apache/hadoop/hive/ql/io/orc/RecordReaderImpl.java | Updated as part of this pull request. |
| ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcStruct.java | Updated as part of this pull request. |
Review details
Suppressed comments (2)
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcStruct.java:605
- This mapping fixes row materialization, but ORC schema inference still unconditionally rejects
TIMESTAMP WITH LOCAL TIME ZONEinOrcSerde.convertPrimitiveType(ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSerde.java:167-169). As a result,CREATE TABLE ... LIKE FILE ORC ...continues to fail for files containing this type, so the ORC read support remains incomplete unless that path is updated or the scope is narrowed.
case TIMESTAMP_INSTANT:
return PrimitiveObjectInspectorFactory.writableTimestampTZObjectInspector;
ql/src/java/org/apache/hadoop/hive/ql/io/orc/WriterImpl.java:224
- This changes observable query behavior: ORC
TIMESTAMP WITH LOCAL TIME ZONEvalues that previously failed or lost data can now round-trip. The PR template defines any user-facing behavioral change as “Yes”, so please update the description’s user-facing-change answer and describe the compatibility impact.
vector.time[rowId] = timestampTZ.toEpochMilli();
vector.nanos[rowId] = timestampTZ.getNanos();
break;
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Requesting @zabetak , @kasakrisz , @soumyakanti3578, @ayushtkn , @deniskuzZ , @dengzhhu653, @saihemanth-cloudera |
cf89476 to
3003925
Compare
|
Handled all the review comments. Requesting PR review @ayushtkn, @saihemanth-cloudera |
asf-ci-hive
added
tests pending
tests unstable
and removed
tests passed
tests pending
labels
|
CI is not green yet. @vikramahuja1001 can you try and get it green? then we can merge this. Thanks. |
…lure on ORC tables
3003925 to
f91e027
Compare
asf-ci-hive
added
tests pending
tests unstable
and removed
tests failed
tests pending
labels
|
What changes were proposed in this pull request?
Fix TIMESTAMP WITH LOCAL TIME ZONE data loss and read failure on ORC tables
Why are the changes needed?
Check https://issues.apache.org/jira/browse/HIVE-30020 for more details
Does this PR introduce any user-facing change?
No
How was this patch tested?
Added a new test case(.q file)