viirya commented on code in PR #7053:
URL: https://github.com/apache/datafusion-comet/pull/7053#discussion_r4362616734
##########
docs/source/user-guide/latest/parquet.md:
##########
@@ -285,17 +285,22 @@ With `spark.comet.write.parquet.enabled=true`:
- **The datetime rebase modes are ignored.** On Spark 3.4 and 3.5, whose
default rebase mode is
`EXCEPTION`, the native writer writes dates and timestamps before October
15, 1582 that Spark
Review Comment:
For timestamps the `EXCEPTION` cutoff on write is 1900-01-01T00:00:00Z, not
1582-10-15. Spark's `WRITE_ANCIENT_DATETIME` message says "writing dates before
1582-10-15 or timestamps before 1900-01-01T00:00:00Z". Could this say dates
before October 15, 1582 and timestamps before January 1, 1900?
##########
spark/src/main/scala/org/apache/comet/serde/operator/CometDataWritingCommand.scala:
##########
@@ -126,6 +127,7 @@ object CometDataWritingCommand extends
CometOperatorSerde[DataWritingCommandExec
.newBuilder()
.setOutputPath(outputPath)
.setCompression(codec)
+ .setSparkVersion(SPARK_VERSION_SHORT)
Review Comment:
Correction to the comment above. On 3.4 through 4.1, Spark's
`ParquetWriteSupport` reads the mode only from
`SQLConf.PARQUET_REBASE_MODE_IN_WRITE`. The `datetimeRebaseMode` option is
read-only. So the check only needs the session conf, which is what
`legacyDatetimeRebaseWriteReason` in #5048 does. For `EXCEPTION`, #7047 already
tracks it, so there's no need for a new issue.
##########
spark/src/main/scala/org/apache/comet/serde/operator/CometWriteFiles.scala:
##########
@@ -135,6 +136,7 @@ object CometWriteFiles extends
CometOperatorSerde[WriteFilesExec] {
val writerOpBuilder = OperatorOuterClass.ParquetWriter
.newBuilder()
.setCompression(codec)
+ .setSparkVersion(SPARK_VERSION_SHORT)
Review Comment:
Same correction as on `CometDataWritingCommand`. Only the session conf
matters here.
##########
native/proto/src/proto/operator.proto:
##########
@@ -1016,6 +1016,11 @@ message ParquetWriter {
// Catalyst's target schema, including top-level nullability and Parquet
field IDs.
// Nested collection field IDs are carried by the corresponding DataType
messages.
repeated SparkStructField output_schema = 9;
+ // The Spark version (`SPARK_VERSION_SHORT`) written into the footer's
key-value metadata as
+ // `org.apache.spark.version`, as Spark's own ParquetWriteSupport does.
Spark's datetime rebase
+ // logic reads it back to tell files written by Spark 3.0+ from older ones.
Absent in plans
Review Comment:
This and @comphead's empty-string comment on `parquet_writer.rs` can be
settled together. Either keep `optional` and skip an empty value before writing
the key, or use a plain `string` where empty means the key is not written, as
#5048 does. Either way, the comment no longer needs the cross-version wording.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
