Skip to content

fix: expand object store option references, uniquify constant metadata names, drop dead parquet JNI - #5653

Merged
sunchao merged 4 commits into
apache:mainfrom
dwsmith1983:fix/scan-core-cleanups
Sep 4, 2026
Merged

fix: expand object store option references, uniquify constant metadata names, drop dead parquet JNI#5653
sunchao merged 4 commits into
apache:mainfrom
dwsmith1983:fix/scan-core-cleanups

Conversation

@dwsmith1983

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Part of the restructuring of #5365 requested in review: these are three small core fixes that were previously bundled into that PR and are independent of the Delta contrib work. The first two are prerequisites for it; the third is standalone cleanup.

Rationale for this change

  1. NativeConfig.extractObjectStoreOptions iterates the Hadoop conf and forwards raw entry values. Configuration#get expands ${...} variable references while Configuration.Entry#getValue does not, so any object store option holding a reference (for example an endpoint composed from another property) reached native as the unexpanded literal and diverged from what every Hadoop-side consumer observes.

  2. Comet appends synthetic constant metadata fields named _comet_metadata_<name> to the scan schema. DataFusion substitutes partition constants by name, so a user or partition column that happens to carry one of those names would silently receive the constant metadata value instead of its own data.

  3. The Java_org_apache_comet_parquet_Native_* record batch reader entry points (initRecordBatchReader, readNextRecordBatch, currentColumnBatch, closeRecordBatchReader) and their BatchContext machinery have no JVM callers anywhere in the tree. Dead code in a JNI surface is worth removing promptly since nothing exercises or protects it.

What changes are included in this PR?

  • extractObjectStoreOptions resolves each value through Configuration#get, falling back to the raw literal when get returns null (deprecated key aliasing) or raises on a substitution cycle, so extraction never aborts.
  • A uniqueConstantMetadataFields helper uniquifies synthetic metadata field names against the data and partition schema names with trailing underscores; binding stays positional so renaming is always safe.
  • Removal of the four dead JNI entry points and their supporting state (306 lines), plus one stale doc comment that referenced them.

How are these changes tested?

  • Two new tests in NativeConfigSuite cover ${...} expansion and the cycle fallback.
  • New CometNativeScanSuite covers prefixing and collision uniquification (written first and failing before the fix).
  • cargo test -p datafusion-comet --lib (240 passed), clippy with -D warnings, fmt, and the Spark 3.5 build with NativeConfigSuite/CometNativeScanSuite all green. A repo-wide grep confirms no callers of the removed JNI symbols.

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 8ffc026d8ea36fdcf08e684e03c93d40285002a8 against 10537e1402716896e9a21344998d9bd05198bcda. The configuration change expands Hadoop references, the metadata helper preserves positional binding, and the removed JNI methods have no remaining tracked callers. One P2 needs addressing: the new scan suite is not registered in the PR build matrices.

Current-head CI has four successful checks, one failed Preflight check and thirteen skipped checks. Preflight stopped at suite registration before the main build and test jobs could run. This is not a failed Spark test. The unchanged checker reproduced the registration failure in a reduced source fixture. Configuration component tests were also run, but no full Spark scan, JNI execution or native build was performed.

import org.apache.spark.sql.catalyst.expressions.AttributeReference
import org.apache.spark.sql.types.{LongType, StringType}

class CometNativeScanSuite extends AnyFunSuite {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Register the new scan suite in both CI matrices

Could you add org.apache.comet.serde.operator.CometNativeScanSuite to the suite lists in both .github/workflows/pr_build_linux.yml and .github/workflows/pr_build_macos.yml? The Preflight job for this head fails at Check missing suites, reports this class missing from the Linux workflow, and exits with code 255. The checker requires registration in both files, and the class is absent from both. This stops Detect changes and leaves the main build and test jobs skipped. Registering only the Linux entry would leave the macOS requirement unsatisfied.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already caught this from the Preflight log and pushed the registration in b234d9c, both workflows. The checker passes locally now.

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed that CometNativeScanSuite is registered in both PR build matrices at b234d9cad0ed4116d9f018b88baf0fbf39dd8b10. The unchanged checker passes against the complete tracked suite set, and the old-head and Linux-only controls reproduce the expected failures. Product source, tests and dependencies are unchanged from the previous review. I have no remaining P1/P2 concerns.

The reviewed CI snapshot still has action_required workflows and no test results for this head. The local registration check is not a full Spark or native build.

Hadoop consumers read conf values through Configuration#get, which
expands ${...} references, while iterator() surfaces the raw literal.
Forward the substituted value so native sees the same credentials,
falling back to the raw literal on null or substitution cycles.
DataFusion substitutes partition constants by name, so a user or
partition column literally named _comet_metadata_<name> would silently
receive the constant metadata value instead of its own. Uniquify the
synthetic names with trailing underscores; binding stays positional.
The Native_initRecordBatchReader / readNextRecordBatch /
currentColumnBatch / closeRecordBatchReader JNI functions and their
BatchContext machinery have no JVM callers. Delete them and update a
comment that cited the removed entry point.
The preflight check requires every suite to be listed in the Linux and
macOS PR build workflows.
@dwsmith1983
dwsmith1983 force-pushed the fix/scan-core-cleanups branch from b234d9c to 9434372 Compare September 3, 2026 09:37
@dwsmith1983
dwsmith1983 requested a review from sunchao September 3, 2026 09:40

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed 943437274cc16a61127cac3c0017c1dd0f47e300 against ef62b463. The PR-only patch and suite-registration fix are unchanged. I checked the incoming shuffle decoder's interaction with the removed Parquet JNI path and constant-field binding and found no new P1/P2. The existing approval stands.

This was a source/integration review using the unchanged prior component and registration evidence. I did not rerun tests or benchmarks. The three current-head workflows still require authorization, with no executed check results reported.

@sunchao
sunchao merged commit 55ae4f2 into apache:main Sep 4, 2026
73 checks passed
@sunchao

sunchao commented Sep 4, 2026

Copy link
Copy Markdown
Member

Merged, thanks @dwsmith1983 !

@dwsmith1983
dwsmith1983 deleted the fix/scan-core-cleanups branch September 4, 2026 03:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants