You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "dwsmith1983 (via GitHub)" <gi...@apache.org> on 2026/09/03 03:24:08 UTC
[PR] fix: expand object store option references, uniquify constant metadata names, drop dead parquet JNI [datafusion-comet]
dwsmith1983 opened a new pull request, #5653:
URL: https://github.com/apache/datafusion-comet/pull/5653
## 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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: expand object store option references, uniquify constant metadata names, drop dead parquet JNI [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao merged PR #5653:
URL: https://github.com/apache/datafusion-comet/pull/5653
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: expand object store option references, uniquify constant metadata names, drop dead parquet JNI [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on PR #5653:
URL: https://github.com/apache/datafusion-comet/pull/5653#issuecomment-5535163071
Merged, thanks @dwsmith1983 !
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: expand object store option references, uniquify constant metadata names, drop dead parquet JNI [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #5653:
URL: https://github.com/apache/datafusion-comet/pull/5653#discussion_r3921176679
##########
spark/src/test/scala/org/apache/comet/serde/operator/CometNativeScanSuite.scala:
##########
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.comet.serde.operator
+
+import org.scalatest.funsuite.AnyFunSuite
+
+import org.apache.spark.sql.catalyst.expressions.AttributeReference
+import org.apache.spark.sql.types.{LongType, StringType}
+
+class CometNativeScanSuite extends AnyFunSuite {
Review Comment:
[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](https://github.com/apache/datafusion-comet/actions/runs/33711222197/job/100522751336) 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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: expand object store option references, uniquify constant metadata names, drop dead parquet JNI [datafusion-comet]
Posted by "dwsmith1983 (via GitHub)" <gi...@apache.org>.
dwsmith1983 commented on code in PR #5653:
URL: https://github.com/apache/datafusion-comet/pull/5653#discussion_r3921314465
##########
spark/src/test/scala/org/apache/comet/serde/operator/CometNativeScanSuite.scala:
##########
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.comet.serde.operator
+
+import org.scalatest.funsuite.AnyFunSuite
+
+import org.apache.spark.sql.catalyst.expressions.AttributeReference
+import org.apache.spark.sql.types.{LongType, StringType}
+
+class CometNativeScanSuite extends AnyFunSuite {
Review Comment:
Already caught this from the Preflight log and pushed the registration in b234d9cad, both workflows. The checker passes locally now.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org