voonhous commented on code in PR #19161:
URL: https://github.com/apache/hudi/pull/19161#discussion_r3563600070
##########
hudi-spark-datasource/hudi-spark/src/main/scala/org/apache/spark/sql/hudi/command/procedures/ExportInstantsProcedure.scala:
##########
@@ -88,19 +88,17 @@ class ExportInstantsProcedure extends BaseProcedure with
ProcedureBuilder with L
if (!new File(localFolder).isDirectory) throw new
HoodieException(localFolder + " is not a valid local directory")
- // The non archived instants can be listed from the Timeline. Wrap in a
mutable list because the
- // desc branch below reverses it in place via Collections.reverse, which
fails on the read-only
- // list produced by asJava over an immutable Scala collection.
- val nonArchivedInstants: util.List[HoodieInstant] = new
util.ArrayList[HoodieInstant](metaClient
+ // The non archived instants can be listed from the Timeline.
+ val nonArchivedInstants: util.List[HoodieInstant] = metaClient
Review Comment:
This is a blocker, not just a question -- confirming hudi-agent's flag above
with evidence. Removing the `new util.ArrayList(...)` wrapper here reverts the
fix from #19172, which is already merged and *is* in this branch's history.
`nonArchivedInstants` (and `archivedStatuses` below) become read-only `asJava`
views over an immutable Scala list, so `Collections.reverse(...)` in the `desc`
branch calls `set()` and throws `UnsupportedOperationException` for any table
with >= 2 matching instants -- i.e. `export_instants(..., desc => true)` breaks
at runtime again, exactly the bug #19172 fixed.
The PR also reverts `TestExportInstantsProcedure` from #19172's four cases
(desc ordering / action filter / invalid folder, with on-disk assertions) back
to a single `result.length == 1` check -- which always passes since the
procedure returns exactly one summary row -- so nothing catches the regression.
Since the description says export_instants coverage lives in #19172, could
you drop both `ExportInstantsProcedure.scala` and
`TestExportInstantsProcedure.scala` from this PR and keep master's versions?
Looks like an accidental over-revert from splitting the export work out.
--
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]
