Repository navigation
[GLUTEN-13256][CH]Support read paimon table in clickhouse backend by using paimon-cpp - #13334
KevinyhZou wants to merge 2 commits into
Conversation
|
Run Gluten Clickhouse CI on x86 |
3 similar comments
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
1 similar comment
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
2 similar comments
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
1 similar comment
|
Run Gluten Clickhouse CI on x86 |
liujiayi771
left a comment
liujiayi771
left a comment
There was a problem hiding this comment.
I mainly reviewed the shared interfaces and JVM-side changes.
| repeated string primary_keys = 9; | ||
| // Native Paimon reader input. The split is DataSplit.serialize() output. | ||
| string table_path = 10; | ||
| bytes serialized_split = 11; |
There was a problem hiding this comment.
Could we preserve the branch and effective read-option overrides from the JVM scan when constructing the paimon-cpp read context?
Currently, only the table path and serialized split are passed through. The reader therefore defaults to the main branch, and query-specific option overrides are not forwarded. Reloading the persisted table options does not necessarily reproduce the JVM scan's read context.
I suggest grouping the SDK input into a dedicated PaimonCppSplit message inside PaimonReadOptions:
message PaimonCppSplit {
bytes serialized_data_split = 1;
string table_path = 2;
string branch = 3;
}
optional PaimonCppSplit cpp_split = 10;Read-option overrides can use the existing properties mechanism, with the reader applying the branch and options via WithBranch() and SetOptions(). Please also add coverage for a non-main branch and a query-level read-option override.
| Integer index, String tablePath, List<byte[]> splits, List<String> preferredLocations) { | ||
| super( | ||
| index, | ||
| java.util.Collections.nCopies(splits.size(), tablePath), |
There was a problem hiding this comment.
Nit: could we add import java.util.Collections; and use Collections.nCopies(...) / Collections.emptyMap() throughout this constructor, consistent with the existing imports and usage of ArrayList, List, and Map?
| case paimonScan: PaimonScan => | ||
| paimonScan.table match { | ||
| case _: FileStoreTable => | ||
| () |
There was a problem hiding this comment.
Could we validate the supported split types before offloading this scan?
Checking only that the table is a FileStoreTable is not sufficient: Paimon 1.4.1 can produce IncrementalSplit for an incremental scan with incremental-between-scan-mode=diff. However, getSplitInfosFromPartitions() below only accepts DataSplit and throws for other split types. That method is called when generating the execution RDD, after plan validation, so this becomes a query failure rather than a JVM fallback.
Please reject unsupported split types or scan modes during validation and add a regression test verifying that an incremental diff scan falls back and returns the expected result. Supporting incremental reads natively can remain a follow-up.
|
|
||
| import org.apache.paimon.spark.read.PaimonLocalScan | ||
|
|
||
| case object CHOffloadPaimonScan extends OffloadPaimonScan { |
There was a problem hiding this comment.
Could CHOffloadPaimonScan extend OffloadSingleNode directly?
It completely overrides offload() and does not reuse the implementation in OffloadPaimonScan. The rule injection already accepts OffloadSingleNode, so inheriting from the concrete Paimon rule does not seem necessary. Extending the interface directly would also let us keep the existing OffloadPaimonScan case class unchanged, avoiding an unrelated change to the shared rule.
|
Run Gluten Clickhouse CI on x86 |
7 similar comments
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
210eb0e to
b3eb25b
Compare
|
Run Gluten Clickhouse CI on x86 |
Add native Paimon scanning to the ClickHouse backend, including build and packaging support and regression tests.
b3eb25b to
902524b
Compare
|
all comments addressed, please help review the code, thanks. @liujiayi771 |
|
Run Gluten Clickhouse CI on x86 |
lgbo-ustc
left a comment
lgbo-ustc
left a comment
There was a problem hiding this comment.
I found two issues in the HDFS adapter and the documented native build path; details are in the inline comments.
| auto hdfs_path = pathPart(path); | ||
| if (hdfsExists(connection_->fs.get(), hdfs_path.c_str()) == 0) | ||
| return true; | ||
| if (errno == ENOENT) |
There was a problem hiding this comment.
[P1] A missing HDFS path is not reliably recognized here. In libhdfs3, hdfsExists() calls FileSystem::exist(), which catches FileNotFoundException and returns false; hdfsExists() then returns -1 without setting errno. If errno is zero or left over from another operation, this returns IOError instead of false. Paimon checks existence of optional paths (e.g. when looking up snapshots), so an ordinary missing path can fail a native HDFS scan. Could we handle the missing-path case without relying on errno after hdfsExists(), and cover Exists(nonexistent_path) in a test?
| export GLUTEN_SOURCE=$(builtin cd $(dirname $0)/../../..; pwd) | ||
| export CH_SOURCE_DIR=${GLUTEN_SOURCE}/cpp-ch/ClickHouse | ||
| CMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE:-RelWithDebInfo} | ||
| GLUTEN_ENABLE_NATIVE_PAIMON=${GLUTEN_ENABLE_NATIVE_PAIMON:-OFF} |
There was a problem hiding this comment.
[P2] This script defaults native Paimon to OFF, whereas the new ClickHouse.md instructions describe -DGLUTEN_ENABLE_NATIVE_PAIMON=ON as the default. Following the documented -Ppaimon JVM build and then invoking this build script without an environment override produces a Paimon-enabled JAR but a libch.so that rejects every native Paimon read with Paimon native read is disabled. Could we align the defaults or show the required GLUTEN_ENABLE_NATIVE_PAIMON=ON invocation explicitly in the docs?
| } | ||
|
|
||
| message PaimonReadOptions { | ||
| message PaimonCppSplit { |
There was a problem hiding this comment.
Could we give this message an implementation-neutral name? It carries a serialized Paimon DataSplit together with the table path, branch, and read options; nothing in the wire format is C++-specific. PaimonCppSplit (and cpp_split below) couples the shared Substrait protocol to the current reader implementation and may be misleading for other consumers. Something like PaimonSplitReadSpec / split_read_spec would describe the payload more accurately; the field number can remain 10.
|
|
||
| import org.apache.spark.util.SparkReflectionUtil | ||
|
|
||
| class CHPaimonComponent extends Component { |
There was a problem hiding this comment.
Could we add a short comment immediately above CHPaimonComponent explaining how it is registered? In particular, src-paimon/main/resources/META-INF/gluten-components/org.apache.gluten.component.CHPaimonComponent is an intentionally empty marker: Gluten discovers the component by the resource file name when the paimon profile packages it, and does not read the file contents. That connection is not apparent from this class or the new marker, so a comment here would help prevent someone from deleting the seemingly unused empty file.
@KevinyhZou No further comments from me on the shared interfaces. Thanks for addressing my feedback! |
|
Run Gluten Clickhouse CI on x86 |
#13256
What changes are proposed in this pull request?
support read pamin table in clickhouse backend
How was this patch tested?
UT
Was this patch authored or co-authored using generative AI tooling?
codex