voonhous commented on code in PR #19815:
URL: https://github.com/apache/hudi/pull/19815#discussion_r3920941197
##########
hudi-common/src/main/java/org/apache/hudi/common/config/HoodieStorageConfig.java:
##########
@@ -240,7 +240,9 @@ public class HoodieStorageConfig extends HoodieConfig {
.key("hoodie.parquet.outputtimestamptype")
.defaultValue("TIMESTAMP_MICROS")
.markAdvanced()
- .withDocumentation("Sets spark.sql.parquet.outputTimestampType. Parquet
timestamp type to use when Spark writes data to Parquet files.");
+ .withDocumentation("Sets spark.sql.parquet.outputTimestampType for the
Spark row-writer Parquet path (for example bulk insert). "
+ + "Writes that go through the Avro path, such as insert and upsert,
derive the Parquet timestamp type from the table's "
+ + "Avro schema and are unaffected by this config.");
Review Comment:
**blocker:** On master the row-writer path does not honor this config
either, so the narrowed description is still incorrect. Since #13882
(4d95b2c2d165, in 1.1.0) `HoodieRowParquetWriteSupport` builds the Parquet
schema itself and takes MICROS/MILLIS from the writer schema's logical type
(`convertField`, L800-816); the value copied into the hadoop conf at L154 is
never read. Verified with a direct write against current snapshot jars:
`TIMESTAMP_MILLIS` with a `timestamp-micros` schema gives `TIMESTAMP(MICROS)`,
and the default with a `timestamp-millis` schema gives `TIMESTAMP(MILLIS)`.
Could we document it as a no-op since 1.1.0 and mark it
`.deprecatedAfter("1.1.0")`, pointing at the writer schema instead?
```suggestion
.withDocumentation("No effect since 1.1.0. Both the Spark row writer
and the Avro Parquet writer derive the "
+ "Parquet timestamp unit from the writer schema's logical type
(timestamp-micros or timestamp-millis); "
+ "declare the precision in the writer schema (for example via
hoodie.write.schema) instead.")
.deprecatedAfter("1.1.0");
```
<details>
<summary>Probe results and why bulk insert used to work</summary>
| config | writer schema | parquet footer |
|---|---|---|
| `TIMESTAMP_MILLIS` | timestamp-micros | `INT64 TIMESTAMP(MICROS,true)` |
| `TIMESTAMP_MICROS` (default) | timestamp-millis | `INT64
TIMESTAMP(MILLIS,true)` |
| `INT96` | timestamp-micros | `INT64 TIMESTAMP(MICROS,true)` |
| unset | timestamp-micros | `INT64 TIMESTAMP(MICROS,true)` |
Each row constructs `HoodieRowParquetWriteSupport` from a `HoodieConfig`
holding `hoodie.avro.schema` plus the config value, writes one `InternalRow`
through `ParquetWriter`, and reads the footer back. Before #13882 the class
extended Spark's `ParquetWriteSupport`, whose `init()` reads
`spark.sql.parquet.outputTimestampType`; that is why bulk insert honored the
config through 1.0.x and the HUDI-8592 reporter saw it work there. Since 1.1.0
it extends parquet's `WriteSupport` directly, so the key is set but never read
on any path.
</details>
##########
hudi-common/src/main/java/org/apache/hudi/common/config/HoodieStorageConfig.java:
##########
@@ -240,7 +240,9 @@ public class HoodieStorageConfig extends HoodieConfig {
.key("hoodie.parquet.outputtimestamptype")
.defaultValue("TIMESTAMP_MICROS")
.markAdvanced()
- .withDocumentation("Sets spark.sql.parquet.outputTimestampType. Parquet
timestamp type to use when Spark writes data to Parquet files.");
+ .withDocumentation("Sets spark.sql.parquet.outputTimestampType for the
Spark row-writer Parquet path (for example bulk insert). "
Review Comment:
**minor:** Not blocking. `HoodieRowParquetWriteSupport.java:154` still does
`hadoopConf.set("spark.sql.parquet.outputTimestampType", ...)` even though
nothing has read it since #13882, and that line is what makes this description
(and the HUDI-8592 triage) look right on a code read.
`HoodieWriteConfig.parquetOutputTimestampType()` has no callers either, as the
PR body notes. Would it be worth dropping both in this PR rather than a
follow-up, so the next reader does not reach the same conclusion?
##########
hudi-common/src/main/java/org/apache/hudi/common/config/HoodieStorageConfig.java:
##########
@@ -240,7 +240,9 @@ public class HoodieStorageConfig extends HoodieConfig {
.key("hoodie.parquet.outputtimestamptype")
.defaultValue("TIMESTAMP_MICROS")
.markAdvanced()
- .withDocumentation("Sets spark.sql.parquet.outputTimestampType. Parquet
timestamp type to use when Spark writes data to Parquet files.");
+ .withDocumentation("Sets spark.sql.parquet.outputTimestampType for the
Spark row-writer Parquet path (for example bulk insert). "
+ + "Writes that go through the Avro path, such as insert and upsert,
derive the Parquet timestamp type from the table's "
+ + "Avro schema and are unaffected by this config.");
Review Comment:
**minor:** Not blocking. Nothing under any `src/test` references
`hoodie.parquet.outputtimestamptype` or `PARQUET_OUTPUT_TIMESTAMP_TYPE`, which
is how the description drifted from the behaviour unnoticed. Could we add a
two-case assertion to `TestHoodieRowParquetWriteSupport`: config
`TIMESTAMP_MILLIS` with a `timestamp-micros` writer schema yields a
`TIMESTAMP(MICROS)` footer, and the default with a `timestamp-millis` schema
yields `TIMESTAMP(MILLIS)`? That pins whatever the description ends up saying.
--
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]
