hudi-agent commented on code in PR #19502:
URL: https://github.com/apache/hudi/pull/19502#discussion_r3710328294
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/BaseCreateHandle.java:
##########
@@ -133,9 +133,9 @@ public void write() {
Iterator<String> keyIterator;
if (hoodieTable.requireSortedRecords()) {
// Sorting the keys limits the amount of extra memory required for
writing sorted records.
- // requireSortedRecords() is true only for HFile base files, which order
keys by UTF-8 bytes,
- // not String (UTF-16) order, so sort with the matching comparator.
- keyIterator =
recordMap.keySet().stream().sorted(StringUtils.UTF8_LEXICOGRAPHIC_COMPARATOR).iterator();
+ keyIterator = recordMap.keySet().stream()
+
.sorted(HoodieRecordUtils.getRecordKeyComparator(hoodieTable.getBaseFileFormat()))
Review Comment:
π€ This effectively flips the physical sort order of Parquet/ORC LSM base
files from UTF-8 bytes (what `requireSortedRecords()` produced before, since
it's been true for LSM layout since #19079) to Java String/UTF-16 order. Reader
and writer now agree, so new tables are fine β but any LSM table already
written by an intermediate build would have UTF-8-ordered base files that the
new UTF-16 merge could mis-order for divergent non-ASCII keys. Since LSM
data-table layout is still unreleased I don't think this blocks, but @yihua
could you confirm there's no data-at-rest we need to account for here?
<sub><i>β οΈ AI-generated; verify before applying. React π/π to flag
quality.</i></sub>
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/HoodieMergeOnReadRDDV2.scala:
##########
@@ -191,18 +192,35 @@ class HoodieMergeOnReadRDDV2(@transient sc: SparkContext,
} else {
val readerContext = new
SparkFileFormatInternalRowReaderContext(fileGroupBaseFileReader.value,
optionalFilters,
Seq.empty, storageConf, metaClient.getTableConfig)
Review Comment:
π€ nit: same near-identical builder duplication as in
`HoodieFileGroupReaderBasedFileFormat` β have you considered pulling the common
`.withβ¦` calls out so the if/else only picks the reader class? Makes it easier
to keep the two paths in sync as the builder evolves.
<sub><i>β οΈ AI-generated; verify before applying. React π/π to flag
quality.</i></sub>
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/HoodieFileGroupReaderBasedFileFormat.scala:
##########
@@ -314,21 +315,42 @@ class HoodieFileGroupReaderBasedFileFormat(tablePath:
String,
} else {
0
}
- val reader = HoodieFileGroupReader.builder()
- .withReaderContext(readerContext)
- .withHoodieTableMetaClient(metaClient)
- .withLatestCommitTime(queryTimestamp)
- .withBaseFileOption(fileSlice.getBaseFile)
- .withLogFiles(fileSlice.getLogFiles)
- .withPartitionPath(fileSlice.getPartitionPath)
- .withDataSchema(dataSchema)
- .withRequestedSchema(requestedSchema)
- .withInternalSchemaOpt(internalSchemaOpt)
- .withProps(props)
- .withStart(file.start)
- .withLength(baseFileLength)
- .withShouldUseRecordPosition(shouldUseRecordPosition)
- .build()
+ val reader: HoodieRecordReader[InternalRow] =
Review Comment:
π€ nit: the two branches share ~12 identical `.withβ¦` builder calls and
differ only on the reader class (and `withShouldUseRecordPosition`). Could you
extract the common builder setup into a small helper (e.g.
`buildReaderBuilder(readerContext, metaClient, β¦)`) so a future `.withFoo(bar)`
addition doesn't have to be applied in two places?
<sub><i>β οΈ AI-generated; verify before applying. React π/π to flag
quality.</i></sub>
--
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]
