hudi-agent commented on code in PR #19866:
URL: https://github.com/apache/hudi/pull/19866#discussion_r3955406861
##########
packaging/hudi-flink-bundle/pom.xml:
##########
@@ -108,6 +108,20 @@
<include>org.apache.parquet:parquet-jackson</include>
<include>com.github.luben:zstd-jni</include>
+ <!-- Lance file I/O uses Arrow C Data, not the dataset or
namespace APIs.
+ Keep Lance and Arrow in their original packages for
JNI. -->
+ <include>org.lance:lance-core</include>
Review Comment:
🤖 `org.lance:lance-core` 4.0.0 is ~162 MB (446 MB unpacked — three platform
native libs, including darwin-aarch64 which is dead weight on a cluster).
Bundling it unconditionally roughly triples the Flink bundle for every user,
including those never touching Lance. Have you considered gating it behind a
profile (like the existing `vortex` profile in hudi-hadoop-common) or a
separate lance bundle, and does the ASF LICENSE/NOTICE need updating for the
bundled native binaries? @danny0405 @yihua wdyt about the release-artifact size
impact here?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
packaging/bundle-validation/validate.sh:
##########
Review Comment:
🤖 **Line 201:** This script has no `set -e`, and only `compact.sh`'s status
is captured into `EXIT_CODE` — so if the Lance job fails (e.g. the exact
`ClassNotFoundException` this PR fixes), the validation still goes green. Could
you check the return code here, e.g. capture `$?` right after and fail the
function on non-zero?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
packaging/bundle-validation/flink/lance.sql:
##########
Review Comment:
🤖 **Line 38:** The smoke test only writes — it never reads back. The read
path (`HoodieRowDataLanceReader` → `LanceFileReader` → `ArrowReader`) pulls in
a different slice of Arrow, and the allowlist here was derived from write-path
ablations, so a class only needed on read could pass CI and still fail users.
Would you mind adding a `SELECT` (ideally with a value assertion) after the
insert?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
packaging/hudi-flink-bundle/pom.xml:
##########
@@ -222,6 +236,19 @@
<pattern>com.fasterxml.jackson.</pattern>
<shadedPattern>${flink.bundle.shade.prefix}com.fasterxml.jackson.</shadedPattern>
</relocation>
+ <!-- Relocate Arrow's Netty buffer patch together with Netty.
-->
+ <relocation>
+ <pattern>io.netty.</pattern>
Review Comment:
🤖 maven-shade rewrites matching string constants too, so Netty's
system-property names become `org.apache.hudi.io.netty.*` inside the bundle —
meaning documented knobs like `-Dio.netty.tryReflectionSetAccessible=true` or
`-Dio.netty.maxDirectMemory` silently stop applying to the bundled allocator.
Did that come up in your JDK 17 testing, and is it worth a note in the
Lance/Flink docs?
<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]
