You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "gengliangwang (via GitHub)" <gi...@apache.org> on 2024/03/18 18:57:37 UTC
[PR] [SPARK-47447][SQL] Allow reading Parquet TimestampLTZ as TimestampNTZ [spark]
gengliangwang opened a new pull request, #45571:
URL: https://github.com/apache/spark/pull/45571
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
Currently, Parquet TimestampNTZ type columns can be read as TimestampLTZ, while reading TimestampLTZ as TimestampNTZ will cause errors. This makes it impossible to read parquet files containing both TimestampLTZ and TimestampNTZ as TimestampNTZ.
To make the data type system on Parquet simpler, this PR allows reading TimestampLTZ as TimestampNTZ in the Parquet data source.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
* Make it possible to read parquet files containing both TimestampLTZ and TimestampNTZ as TimestampNTZ
* Make the data type system on Parquet simpler
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
Yes, Parquet TimestampLTZ type column are now allowed to be read as TimestampNTZ
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
UT
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47447][SQL] Allow reading Parquet TimestampLTZ as TimestampNTZ [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #45571:
URL: https://github.com/apache/spark/pull/45571#discussion_r1530949174
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetRowConverter.scala:
##########
@@ -436,6 +436,17 @@ private[parquet] class ParquetRowConverter(
}
}
+ // INT96 timestamp doesn't have a logical type, here we check the physical type instead.
+ case TimestampNTZType if parquetType.asPrimitiveType().getPrimitiveTypeName == INT96 =>
+ new ParquetPrimitiveConverter(updater) {
+ // Converts nanosecond timestamps stored as INT96.
+ // TimestampNTZ type does not require rebasing due to its lack of time zone context.
Review Comment:
* LTZ doesn't really store the time zone info in Parquet files. Also, Spark uses the long value directly when reading NTZ as LTZ. I am trying to make it simple and Symmetrical.
* If we shift by sessional time zone here, probably we need to do it when reading NTZ as LTZ, which will be a breaking change. Also, the result of NTZ columns will be affected by the sessional time zone conf `spark.sql.session.timeZone`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47447][SQL] Allow reading Parquet TimestampLTZ as TimestampNTZ [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #45571:
URL: https://github.com/apache/spark/pull/45571#issuecomment-2005670051
Could you explain why we make a difference in the way Parquet and JDBC handle LTZ? As for JDBC LTZ, this kind of conversion is forbidden if we know we are reading an LTZ value
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47447][SQL] Allow reading Parquet TimestampLTZ as TimestampNTZ [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang closed pull request #45571: [SPARK-47447][SQL] Allow reading Parquet TimestampLTZ as TimestampNTZ
URL: https://github.com/apache/spark/pull/45571
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47447][SQL] Allow reading Parquet TimestampLTZ as TimestampNTZ [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #45571:
URL: https://github.com/apache/spark/pull/45571#discussion_r1530167583
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetRowConverter.scala:
##########
@@ -436,6 +436,17 @@ private[parquet] class ParquetRowConverter(
}
}
+ // INT96 timestamp doesn't have a logical type, here we check the physical type instead.
+ case TimestampNTZType if parquetType.asPrimitiveType().getPrimitiveTypeName == INT96 =>
+ new ParquetPrimitiveConverter(updater) {
+ // Converts nanosecond timestamps stored as INT96.
+ // TimestampNTZ type does not require rebasing due to its lack of time zone context.
Review Comment:
Could you tell me why we can treat ltz as ntz directly?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47447][SQL] Allow reading Parquet TimestampLTZ as TimestampNTZ [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on PR #45571:
URL: https://github.com/apache/spark/pull/45571#issuecomment-2005751959
@yaooqinn For parquet files, it's possible that a file directory contains two different kinds of timestamp columns. Before this change, only timestamp type can be used to read such a file directory. While for JDBC, a table type is certain.
But you make a good point here. I am also considering allowing reading INT96 as TimestampNTZ, since there is no clear definition of this Parquet physical type. We can still fobid reading Int64 timestamps with `isAdjustedToUTC=true` as TimestampNTZ.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47447][SQL] Allow reading Parquet TimestampLTZ as TimestampNTZ [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on PR #45571:
URL: https://github.com/apache/spark/pull/45571#issuecomment-2008710154
@yaooqinn @cloud-fan @beliefer Thanks for the review. I am merging this one to master.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47447][SQL] Allow reading Parquet TimestampLTZ as TimestampNTZ [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on PR #45571:
URL: https://github.com/apache/spark/pull/45571#issuecomment-2004700401
cc @sadikovi @srielau too
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47447][SQL] Allow reading Parquet TimestampLTZ as TimestampNTZ [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #45571:
URL: https://github.com/apache/spark/pull/45571#issuecomment-2005751742
due to schema evolution, it's very likely to have mixed data types in parquet files. e.g. turn on NTZ, write some files, turn it off (maybe hit some issues), write some files, and turn it on again (issue fixed), read some files.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47447][SQL] Allow reading Parquet TimestampLTZ as TimestampNTZ [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #45571:
URL: https://github.com/apache/spark/pull/45571#discussion_r1531453686
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetRowConverter.scala:
##########
@@ -436,6 +436,17 @@ private[parquet] class ParquetRowConverter(
}
}
+ // INT96 timestamp doesn't have a logical type, here we check the physical type instead.
+ case TimestampNTZType if parquetType.asPrimitiveType().getPrimitiveTypeName == INT96 =>
+ new ParquetPrimitiveConverter(updater) {
+ // Converts nanosecond timestamps stored as INT96.
+ // TimestampNTZ type does not require rebasing due to its lack of time zone context.
Review Comment:
Got it. Thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47447][SQL] Allow reading Parquet TimestampLTZ as TimestampNTZ [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #45571:
URL: https://github.com/apache/spark/pull/45571#issuecomment-2005809344
"Understood. Thank you for explaining that, @gengliangwang."
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org