Initial PR #1

sunchao · 2024-01-24T18:24:59Z

This is the initial PR for Comet.

Related mailing list discussion: https://lists.apache.org/thread/0q1rb11jtpopc7vt1ffdzro0omblsh0s

kazuyukitanimura

LGTM (disclosure: I am one of the authors) Thank you @sunchao

alamb · 2024-01-24T20:27:19Z

Thank you @sunchao -- I plan to give this a review over the next day or two

Co-authored-by: Liang-Chi Hsieh <[email protected]> Co-authored-by: Kazuyuki Tanimura <[email protected]> Co-authored-by: Steve Vaughan Jr <[email protected]> Co-authored-by: Huaxin Gao <[email protected]> Co-authored-by: Parth Chandra <[email protected]> Co-authored-by: Oleksandr Voievodin <[email protected]>

andygrove · 2024-01-24T23:40:52Z

I was able to build the project and run some queries successfully. I plan on reviewing this over the weekend.

andygrove · 2024-01-25T00:29:25Z

rust-toolchain

@@ -0,0 +1 @@
+nightly-2023-09-05


I'm curious to know why nightly Rust is required. It would be good to add some docs on this at some point.

Yes, at this time it requires nightly Rust to compile. We started with stable Rust but at some point introduced some nightly-only features like "specialization". I think it is very easy to remove the dependency though - we can switch back to stable Rust later.

alamb

I took a look at this code (obviously not the whole thing in detail) and I thought it was pretty awesome ❤️

The code I looked at looks clear, well commented and well tested.

I wonder if you have a public roadmap about where you hope to take this project?

As I understand it the next step is to perform the IP clearance process https://incubator.apache.org/ip-clearance/ (I can help with this if you need as I did it for the object_store donation).

Once the IP clearance process is complete, I think this would make a great part of the apache arrow datafusion project

Some notes I found interesting while reviewing:

There appears to be another implementation of parquet in java as well as in rust.
There is a set of kernels (e.g. core/src/execution/kernels/strings.rs that seems somewhat similar to what is in arrow-rs and datafusion)
The docs imply there is codgen for filters, but I didn't find any reference to that in the code

alamb · 2024-01-25T17:49:36Z

common/src/main/java/org/apache/comet/parquet/BloomFilterReader.java

+import org.apache.parquet.hadoop.metadata.ColumnPath;
+import org.apache.parquet.io.SeekableInputStream;
+
+public class BloomFilterReader implements FilterPredicate.Visitor<Boolean> {


FWIW DataFusion's parquet reader handle bloom filters natively now thanks to @hengfeiyang https://github.com/apache/arrow-datafusion/blob/5e9c9a1f7cecabe6e6c40c8296adb517fac0da13/datafusion/core/src/datasource/physical_plan/parquet/row_groups.rs#L113

Though I don't think it supports encrypted ciphers

That's great to know :)

I need to check a list of things that are in Parquet Java but not in the Rust yet. I think the Parquet encryption is definitely an important one.

alamb · 2024-01-25T17:51:50Z

common/src/main/java/org/apache/comet/parquet/AbstractColumnReader.java

@@ -0,0 +1,116 @@
+/*


I am fascinated to know (can be later) why comet needs its own parquet reader in Java -- maybe we can add any missing functionality upstream in parquet-rs

Yes, when we started there are several things that are not yet ready in the Rust implementation yet, so we chose to use this hybrid implementation. The Rust implementation definitely has become much more mature now, and we do want to switch to it at some point.

I think to check what are the things that are missing in the Rust side. Perhaps:

Parquet encryption support

Check all the predicates and see if they are supported (e.g., in/notIn?)

Dictionary pushdown? maybe it is already supported.

We also needed to do a bunch of Spark-specific things in our native Parquet reader. For instance, Spark has this timestamp/date rebase feature for conversions from the old Julian calendar to Gregorian calendar, and it also reads small precision decimal into i32 or i64 on the Java side, which requires special handling.

alamb · 2024-01-25T18:02:21Z

core/src/execution/mod.rs

+}
+
+#[cfg(test)]
+mod tests {


alamb · 2024-01-25T18:03:31Z

core/src/execution/kernels/temporal.rs

+    Some(dt).and_then(|d| d.with_nanosecond(1_000 * (d.nanosecond() / 1_000)))
+}
+
+pub fn date_trunc_dyn(array: &dyn Array, format: String) -> Result<ArrayRef, ExpressionError> {


FWIW over time I hope we can move most functions like date_trunc out of DataFusion's core and potentially have versions like this with spark compatible behavior available for others to use and help maintain

Definitely!

This is what I have in mind, BTW, in case anyone has time to review: apache/datafusion#8705

sunchao · 2024-01-25T22:25:25Z

Thanks @alamb , really appreciated

I wonder if you have a public roadmap about where you hope to take this project?

We don't have it yet. Internally we do have roadmap under doc but it was removed in this PR. We can add it back after the initial PR.

As I understand it the next step is to perform the IP clearance process ...

That's great! I'll check how it was done for other projects, and let you know if I need any help with it.

There appears to be another implementation of parquet in java as well as in rust.

Yes, the Comet Parquet reader is a hybrid implementation: the IO part is done in Java while the decoding (to Arrow) & decompression is done in native. This is based on the assumption that we won't get much performance gain by moving the IO part to native. While keeping it in Java, we are able to leverage various storage connectors such as S3 and HDFS, that are already pretty mature, as well as Parquet features that are missing on the native side, like encryption support.

With that said, at some point we do want to switch to a fully native Parquet reader like the one in DF. This can potentially help to simplify a lot of the logic we currently have.

There is a set of kernels (e.g. core/src/execution/kernels/strings.rs that seems somewhat similar to what is in arrow-rs and datafusion)

Yes, I think we should be able to switch to the ones in DF now. These were added long time back when some of the string kernels in DF still didn't support dictionary, which is no longer true.

The docs imply there is codgen for filters, but I didn't find any reference to that in the code

This is something we want to do in Comet, but hasn't started yet :)

liurenjie1024 · 2024-01-26T12:30:48Z

Thanks @sunchao for this contribution, very great work! Just curious if there is any performance report compared with vanilla spark?

sunchao · 2024-01-26T18:23:28Z

Hey @liurenjie1024 , we haven't done TPC-H/TPC-DS benchmark recently since there are still some important features that are missing, such as join support (which we are working on at the moment). We plan to run these benchmarks once the coverage is better and publish the results in the repo. For TPC-H q01 which we do support most operators, I think we saw 5x+ improvements (it can definitely go higher with further optimizations).

liurenjie1024 · 2024-01-27T01:30:56Z

Hey @liurenjie1024 , we haven't done TPC-H/TPC-DS benchmark recently since there are still some important features that are missing, such as join support (which we are working on at the moment). We plan to run these benchmarks once the coverage is better and publish the results in the repo. For TPC-H q01 which we do support most operators, I think we saw 5x+ improvements (it can definitely go higher with further optimizations).

That's awesome!

andygrove · 2024-01-27T15:33:04Z

I have spent some time looking at the code and found it to be well-written and easy to navigate. As I previously mentioned, I was able to run some queries and see performance improvements over regular Spark, so this LGTM as a donation.

I believe that the next step is to have a formal vote on accepting this donation, and we will need to link to that mailing list discussion as part of the IP clearance process.

I have created a Google document where the contributors can fill out the information needed to start the IP clearance process.

https://docs.google.com/document/d/1azmxE1LERNUdnpzqDO5ortKTsPKrhNgQC4oZSmXa8x4/edit?usp=sharing

andygrove · 2024-01-27T16:11:52Z

Mailing list thread for the vote: https://lists.apache.org/thread/sk70pkhwmt8vgn0thtr04qg4mpqsgfvx

kou · 2024-01-27T22:05:30Z

Can we check RAT https://creadur.apache.org/rat/ result?
For example, apache/arrow-rs uses https://github.com/apache/arrow-rs/blob/master/dev/release/run-rat.sh to run RAT.

viirya · 2024-01-27T23:34:36Z

I manually run the script on this PR.

NOT APPROVED: .gitignore (./.gitignore): false
NOT APPROVED: Makefile (./Makefile): false
NOT APPROVED: filtered_rat.txt (./filtered_rat.txt): false
NOT APPROVED: rat.txt (./rat.txt): false
NOT APPROVED: rat_exclude_files.txt (./rat_exclude_files.txt): false
NOT APPROVED: rust-toolchain (./rust-toolchain): false
NOT APPROVED: core/.lldbinit (./core/.lldbinit): false
NOT APPROVED: core/Cargo.lock (./core/Cargo.lock): false
NOT APPROVED: spark/src/test/resources/tpch-extended/q1.sql (./spark/src/test/resources/tpch-extended/q1.sql): false
       9 unapproved licences. Check rat report: rat.txt

filtered_rat.txt, rat.txt, rat_exclude_files.txt are the files related to the RAT check script.
q1.sql is SQL test query file. In DataFusion, these files don't have license header too.
Cargo.lock is automatically generated by Cargo.
Seems that we don't need to add license header to rust-toolchain.

I think core/.lldbinit is debugger's config file and committed wrongly. Removed it.

Only missing license header is Makefile. I just added it.

sunchao · 2024-01-28T00:15:10Z

Thanks @viirya !

I think core/.lldbinit is debugger's config file and committed wrongly. Removed it.

I think this is a sample file. It is mentioned in DEBUGGING.md

viirya · 2024-01-28T00:53:43Z

I think this is a sample file. It is mentioned in DEBUGGING.md

Oh, got it. I removed it for now and updated DEBUGGING.md. If we need it, we can add it back later. Thanks.

kou · 2024-01-28T02:24:58Z

Thanks!

alamb · 2024-01-28T13:09:55Z

In DataFusion, these files don't have license header too.

I think we have a list of files that are excluded from the RAT check -- specifically https://github.com/apache/arrow-datafusion/blob/main/dev/release/rat_exclude_files.txt

advancedxy · 2024-01-29T03:10:15Z

This is awesome and exciting. Just curious to know how many(percent maybe) internal workloads have already on this one, if it's ok to share it publicly?

I think I can help/contribute a bit to fill the semantic gaps between this and vanilla spark if needed.

sunchao · 2024-01-29T18:44:02Z

@advancedxy thanks for the interest! it will be great to collaborate with you on this :)

All of our Spark 3.4 production workloads are already using Comet, although only the native Parquet scan feature at the moment. We are finishing up some necessary work including things such as columnar shuffle support and unified memory management, before rolling out more features to our customers.

advancedxy · 2024-01-31T09:33:32Z

All of our Spark 3.4 production workloads are already using Comet, although only the native Parquet scan feature at the moment.

Thanks for sharing, I think this is a smart strategy to roll out migrations incrementally like this.

lorentzenchr · 2024-02-01T21:14:48Z

README.md

+# Apache Arrow DataFusion Comet
+
+Comet is an Apache Spark plugin that uses [Apache Arrow DataFusion](https://arrow.apache.org/datafusion/)
+as native runtime to achieve dramatic improvement in terms of query efficiency and query runtime.


„dramatic“ seems a bit too dramatic😉

BTW, if it’s allowed to disclose, which companies are behind the development of comet?

Yes it is. We should remove this word for now.

The initial contributors are from Apple (as can be seen from the PR), but we'd love to collaborate with people from the open source community who wants to achieve similar goals.

andygrove · 2024-02-03T18:22:31Z

The vote to accept the donation has passed and the next step is to complete the IP clearance process.

I have started filling out the XML IP clearance form in #2

andygrove · 2024-02-03T20:12:24Z

License check for the Rust dependencies:

$ cargo license
(MIT OR Apache-2.0) AND Unicode-DFS-2016 (1): unicode-ident
0BSD OR Apache-2.0 OR MIT (1): adler
Apache-2.0 (40): arrow, arrow-arith, arrow-array, arrow-buffer, arrow-cast, arrow-csv, arrow-data, arrow-ipc, arrow-json, arrow-ord, arrow-row, arrow-schema, arrow-select, arrow-string, ciborium, ciborium-io, ciborium-ll, datafusion, datafusion-common, datafusion-execution, datafusion-expr, datafusion-optimizer, datafusion-physical-expr, datafusion-physical-plan, datafusion-sql, debugid, flatbuffers, parquet, parquet-format, pprof, prost, prost, prost-build, prost-derive, prost-derive, prost-types, sqlparser, sqlparser_derive, thrift, thrift
Apache-2.0 OR Apache-2.0 WITH LLVM-exception OR MIT (3): linux-raw-sys, rustix, wasi
Apache-2.0 OR BSD-2-Clause OR MIT (2): zerocopy, zerocopy-derive
Apache-2.0 OR BSL-1.0 (1): ryu
Apache-2.0 OR CC0-1.0 (1): blake3
Apache-2.0 OR CC0-1.0 OR MIT-0 (1): constant_time_eq
Apache-2.0 OR GPL-2.0 OR GPL-3.0 OR MIT (1): assertables
Apache-2.0 OR MIT (200): addr2line, ahash, allocator-api2, android-tzdata, android_system_properties, anes, anstyle, anyhow, arc-swap, arrayvec, async-trait, autocfg, backtrace, base64, bitflags, bitflags, blake2, block-buffer, bumpalo, cast, cc, cesu8, cfg-if, chrono, chrono-tz, chrono-tz-build, clap, clap_builder, clap_lex, const-random, const-random-macro, core-foundation-sys, cpp_demangle, cpufeatures, crc32fast, criterion, criterion-plot, crossbeam-deque, crossbeam-epoch, crossbeam-utils, crypto-common, derivative, destructure_traitobject, digest, either, equivalent, errno, fastrand, findshlibs, fixedbitset, flate2, fnv, form_urlencoded, futures, futures-channel, futures-core, futures-executor, futures-io, futures-macro, futures-sink, futures-task, futures-util, getrandom, gimli, glob, half, half, hashbrown, hashbrown, heck, heck, hermit-abi, hex, home, humantime, iana-time-zone, iana-time-zone-haiku, idna, indexmap, indexmap, itertools, itertools, itertools, itoa, jni, jni-sys, jobserver, js-sys, lazy_static, lexical-core, lexical-parse-float, lexical-parse-integer, lexical-util, lexical-write-float, lexical-write-integer, libc, libm, linked-hash-map, lock_api, log, log-mdc, log4rs, md-5, memmap2, multimap, num, num-bigint, num-complex, num-format, num-integer, num-iter, num-rational, num-traits, num_cpus, object, object_store, once_cell, parking_lot, parking_lot_core, paste, percent-encoding, petgraph, pin-project-lite, pin-utils, pkg-config, ppv-lite86, proc-macro2, quote, rand, rand_chacha, rand_core, rayon, rayon-core, regex, regex-automata, regex-syntax, rustc-demangle, rustc_version, rustversion, scopeguard, semver, seq-macro, serde, serde_derive, serde_json, serde_yaml, sha2, siphasher, smallvec, snafu, snafu-derive, stable_deref_trait, static_assertions, str_stack, syn, syn, tempfile, thiserror, thiserror-impl, thread-id, threadpool, tinytemplate, typenum, unicode-bidi, unicode-normalization, unicode-segmentation, unicode-width, url, uuid, version_check, wasm-bindgen, wasm-bindgen-backend, wasm-bindgen-macro, wasm-bindgen-macro-support, wasm-bindgen-shared, web-sys, winapi, winapi-i686-pc-windows-gnu, winapi-x86_64-pc-windows-gnu, windows-core, windows-sys, windows-targets, windows-targets, windows_aarch64_gnullvm, windows_aarch64_gnullvm, windows_aarch64_msvc, windows_aarch64_msvc, windows_i686_gnu, windows_i686_gnu, windows_i686_msvc, windows_i686_msvc, windows_x86_64_gnu, windows_x86_64_gnu, windows_x86_64_gnullvm, windows_x86_64_gnullvm, windows_x86_64_msvc, windows_x86_64_msvc, yaml-rust, zstd-safe, zstd-sys
Apache-2.0 OR MIT OR Zlib (4): bytemuck, miniz_oxide, tinyvec, tinyvec_macros
BSD-2-Clause (1): arrayref
BSD-3-Clause (4): alloc-no-stdlib, alloc-stdlib, snap, subtle
BSD-3-Clause OR MIT (2): brotli, brotli-decompressor
CC0-1.0 (1): tiny-keccak
CDDL-1.0 (1): inferno
MIT (46): bytes, combine, comfy-table, crunchy, dashmap, doc-comment, generic-array, integer-encoding, integer-encoding, is-terminal, lz4, lz4-sys, nix, oorandom, ordered-float, ordered-float, parse-zoneinfo, phf, phf_codegen, phf_generator, phf_shared, plotters, plotters-backend, plotters-svg, quick-xml, redox_syscall, rgb, serde-value, simd-adler32, slab, strum, strum_macros, symbolic-common, symbolic-demangle, tokio, tokio-macros, tokio-stream, tokio-util, tracing, tracing-attributes, tracing-core, twox-hash, typemap-ors, unsafe-any-ors, which, zstd
MIT OR Unlicense (8): aho-corasick, byteorder, csv, csv-core, memchr, same-file, walkdir, winapi-util
N/A (1): comet

could you add https://github.com/apache/arrow-datafusion/blob/main/LICENSE.txt to the root of the repo in the PR?

andygrove · 2024-02-03T20:15:14Z

I manually checked the maven dependencies are licenses are all good.

sunchao · 2024-02-03T20:15:46Z

could you add https://github.com/apache/arrow-datafusion/blob/main/LICENSE.txt to the root of the repo in the PR?

Sure @andygrove, just added the LICENSE.txt

alamb · 2024-02-04T11:29:38Z

Update: moved grant discussion https://github.com/apache/arrow-datafusion-comet/pull/2/files#r1477313278

andygrove · 2024-02-07T00:11:31Z

I have started the IP clearance vote: https://lists.apache.org/thread/lj3j4q7snpzrfo3mh3cph26mdpr2jrfx

EpsilonPrime

I glanced through the PR and am excited to see it being shared with the open source community. May the project flourish!

EpsilonPrime · 2024-01-24T20:44:28Z

common/src/main/java/org/apache/comet/NativeBase.java

+      System.setProperty(key, value);
+    } else {
+      LOG.info(
+          "Skip setting system property {} to {}, because it is already set to {}",


EpsilonPrime · 2024-01-24T20:47:53Z

common/src/main/java/org/apache/comet/parquet/ColumnIndexReader.java

+        Util.readColumnIndex(inputStream, columnIndexDecryptor, columnIndexAAD));
+  }
+
+  // Visible for testing


Can this comment be replaced with an annotation?

We need to check. I remember we did this explicitly to avoid some additional dependency.

andygrove · 2024-02-09T23:42:48Z

I have started the IP clearance vote: https://lists.apache.org/thread/lj3j4q7snpzrfo3mh3cph26mdpr2jrfx

The IP clearance vote has passed.

viirya · 2024-02-09T23:59:36Z

Thanks all for the help on this!

alamb · 2024-02-11T10:57:05Z

Nice work -- so excited!

add partial support for multiple parquet files

sunchao changed the title ~~Initial PR for Comet donation~~ Initial PR Jan 24, 2024

sunchao force-pushed the comet-upstream branch from 0ad0ba7 to 3feecfe Compare January 24, 2024 18:36

kazuyukitanimura approved these changes Jan 24, 2024

View reviewed changes

sunchao force-pushed the comet-upstream branch from 3feecfe to 2b95ac4 Compare January 24, 2024 20:37

andygrove reviewed Jan 25, 2024

View reviewed changes

alamb mentioned this pull request Jan 25, 2024

DataFusion weekly project plan (Andrew Lamb) - Jan 22, 2024 apache/datafusion#8933

Closed

9 tasks

alamb reviewed Jan 25, 2024

View reviewed changes

alamb mentioned this pull request Jan 26, 2024

Relax join keys constraint from Column to any physical expression for physical join operators apache/datafusion#8991

Merged

Add license header to Makefile. Remove unncessary file core/.lldbinit.

5553fb7

Update DEBUGGING.md

ce7852e

alamb mentioned this pull request Jan 29, 2024

[EPIC] Support Exprs as equality join predicates (constants) rather than just Columns in physical joins apache/datafusion#9056

Open

2 tasks

lorentzenchr reviewed Feb 1, 2024

View reviewed changes

add license and address comments

408dbe3

alamb mentioned this pull request Feb 6, 2024

IP clearance form #2

Closed

EpsilonPrime reviewed Feb 7, 2024

View reviewed changes

andygrove approved these changes Feb 9, 2024

View reviewed changes

andygrove merged commit 383c8fd into main Feb 9, 2024

alamb deleted the comet-upstream branch February 11, 2024 10:57

This was referenced Feb 11, 2024

Write a blog post about the comet donation #6

Closed

DataFusion weekly project plan (Andrew Lamb) - Feb 12, 2024 apache/datafusion#9200

Closed

dpengpeng mentioned this pull request Jun 21, 2024

Comet can't use CometShuffleManager on Yarn Cluster #592

Closed

andygrove pushed a commit to andygrove/datafusion-comet that referenced this pull request Nov 9, 2024

Merge pull request apache#1 from andygrove/df-parquet-exec

ef9f8f5

add partial support for multiple parquet files

		@@ -0,0 +1 @@
		nightly-2023-09-05

Initial PR #1

Initial PR #1

Conversation

sunchao commented Jan 24, 2024 • edited by alamb Loading

kazuyukitanimura left a comment

Choose a reason for hiding this comment

alamb commented Jan 24, 2024

andygrove commented Jan 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunchao Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunchao commented Jan 25, 2024

liurenjie1024 commented Jan 26, 2024

sunchao commented Jan 26, 2024

liurenjie1024 commented Jan 27, 2024

andygrove commented Jan 27, 2024

andygrove commented Jan 27, 2024

kou commented Jan 27, 2024

viirya commented Jan 27, 2024 • edited Loading

sunchao commented Jan 28, 2024

viirya commented Jan 28, 2024

kou commented Jan 28, 2024

alamb commented Jan 28, 2024

advancedxy commented Jan 29, 2024

sunchao commented Jan 29, 2024 • edited Loading

advancedxy commented Jan 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andygrove commented Feb 3, 2024

andygrove commented Feb 3, 2024

andygrove commented Feb 3, 2024

sunchao commented Feb 3, 2024

alamb commented Feb 4, 2024 • edited Loading

andygrove commented Feb 7, 2024

EpsilonPrime left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andygrove commented Feb 9, 2024

viirya commented Feb 9, 2024

alamb commented Feb 11, 2024

sunchao commented Jan 24, 2024 •

edited by alamb

Loading

sunchao Jan 25, 2024 •

edited

Loading

viirya commented Jan 27, 2024 •

edited

Loading

sunchao commented Jan 29, 2024 •

edited

Loading

alamb commented Feb 4, 2024 •

edited

Loading