You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "sunchao (via GitHub)" <gi...@apache.org> on 2024/01/24 18:25:01 UTC
[PR] Initial PR for Comet donation [arrow-datafusion-comet]
sunchao opened a new pull request, #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1
This is the initial PR for donating Comet to ASF under Apache Arrow DataFusion.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#issuecomment-1925711918
When we donated the `object_store` crate initially to ASF, we also filled submitted a Software Grant: https://www.apache.org/licenses/contributor-agreements.html#grants
> When an individual or corporation decides to donate a body of existing software or documentation to one of the Apache projects, they need to execute a formal [Software Grant Agreement](https://www.apache.org/licenses/software-grant-template.pdf) (SGA) with the ASF. Typically, they do this after negotiating approval with the ASF [Incubator](http://incubator.apache.org/) or one of the PMCs, since the ASF does not accept software unless there is a viable community available to support it as part of a collaborative project.
We already have a vote to accept this software into the Arrow project by the Arrow PMC.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#issuecomment-1913381014
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](https://github.com/apache/arrow-datafusion-comet/pull/1/files#diff-0d9dc885c7209bb85b2ae380ad78b6e37bf8009e40dbafe51e47082ca118b495R36)
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#issuecomment-1913187836
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
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#issuecomment-1911097541
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](https://github.com/apache/arrow-rs/issues/3511).
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](https://github.com/apache/arrow-datafusion-comet/blob/comet-upstream/README.md) 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 :)
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#issuecomment-1913591538
> 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
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#discussion_r1467031959
##########
common/src/main/java/org/apache/comet/parquet/BloomFilterReader.java:
##########
@@ -0,0 +1,253 @@
+/*
+ * 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.parquet;
+
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.parquet.column.values.bloomfilter.BlockSplitBloomFilter;
+import org.apache.parquet.column.values.bloomfilter.BloomFilter;
+import org.apache.parquet.crypto.AesCipher;
+import org.apache.parquet.crypto.InternalColumnDecryptionSetup;
+import org.apache.parquet.crypto.InternalFileDecryptor;
+import org.apache.parquet.crypto.ModuleCipherFactory;
+import org.apache.parquet.crypto.ParquetCryptoRuntimeException;
+import org.apache.parquet.filter2.predicate.FilterPredicate;
+import org.apache.parquet.filter2.predicate.Operators;
+import org.apache.parquet.filter2.predicate.UserDefinedPredicate;
+import org.apache.parquet.format.BlockCipher;
+import org.apache.parquet.format.BloomFilterHeader;
+import org.apache.parquet.format.Util;
+import org.apache.parquet.hadoop.metadata.BlockMetaData;
+import org.apache.parquet.hadoop.metadata.ColumnChunkMetaData;
+import org.apache.parquet.hadoop.metadata.ColumnPath;
+import org.apache.parquet.io.SeekableInputStream;
+
+public class BloomFilterReader implements FilterPredicate.Visitor<Boolean> {
Review Comment:
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.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#issuecomment-1913391768
> I think this is a sample file. It is mentioned in [DEBUGGING.md](https://github.com/apache/arrow-datafusion-comet/pull/1/files#diff-0d9dc885c7209bb85b2ae380ad78b6e37bf8009e40dbafe51e47082ca118b495R36)
Oh, got it. I removed it for now and updated DEBUGGING.md. If we need it, we can add it back later. Thanks.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#issuecomment-1913225844
Mailing list thread for the vote: https://lists.apache.org/thread/sk70pkhwmt8vgn0thtr04qg4mpqsgfvx
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "lorentzenchr (via GitHub)" <gi...@apache.org>.
lorentzenchr commented on code in PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#discussion_r1475157229
##########
README.md:
##########
@@ -1 +1,60 @@
-# arrow-datafusion-comet
+<!--
+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.
+-->
+
+# 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.
Review Comment:
„dramatic“ seems a bit too dramatic😉
BTW, if it’s allowed to disclose, which companies are behind the development of comet?
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#discussion_r1467039101
##########
common/src/main/java/org/apache/comet/parquet/AbstractColumnReader.java:
##########
@@ -0,0 +1,116 @@
+/*
Review Comment:
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](https://github.com/apache/parquet-mr/blob/master/parquet-column/src/main/java/org/apache/parquet/filter2/predicate/FilterApi.java) 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.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#issuecomment-1909105863
I was able to build the project and run some queries successfully. I plan on reviewing this over the weekend.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#issuecomment-1936744383
> I have started the IP clearance vote: https://lists.apache.org/thread/lj3j4q7snpzrfo3mh3cph26mdpr2jrfx
The IP clearance vote has passed.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#issuecomment-1911998346
Thanks @sunchao for this contribution, very great work! Just curious if there is any performance report compared with vanilla spark?
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#discussion_r1465710384
##########
rust-toolchain:
##########
@@ -0,0 +1 @@
+nightly-2023-09-05
Review Comment:
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.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#discussion_r1467039101
##########
common/src/main/java/org/apache/comet/parquet/AbstractColumnReader.java:
##########
@@ -0,0 +1,116 @@
+/*
Review Comment:
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 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](https://github.com/apache/parquet-mr/blob/master/parquet-column/src/main/java/org/apache/parquet/filter2/predicate/FilterApi.java) 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.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#issuecomment-1925421425
The [vote to accept the donation](https://lists.apache.org/thread/sk70pkhwmt8vgn0thtr04qg4mpqsgfvx) has passed and the next step is to complete the IP clearance process.
I have started filling out the XML IP clearance form in https://github.com/apache/arrow-datafusion-comet/pull/2
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "EpsilonPrime (via GitHub)" <gi...@apache.org>.
EpsilonPrime commented on code in PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#discussion_r1465519630
##########
common/src/main/java/org/apache/comet/parquet/ColumnIndexReader.java:
##########
@@ -0,0 +1,230 @@
+/*
+ * 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.parquet;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Set;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.parquet.crypto.AesCipher;
+import org.apache.parquet.crypto.InternalColumnDecryptionSetup;
+import org.apache.parquet.crypto.InternalFileDecryptor;
+import org.apache.parquet.crypto.ModuleCipherFactory;
+import org.apache.parquet.format.BlockCipher;
+import org.apache.parquet.format.Util;
+import org.apache.parquet.format.converter.ParquetMetadataConverter;
+import org.apache.parquet.hadoop.metadata.BlockMetaData;
+import org.apache.parquet.hadoop.metadata.ColumnChunkMetaData;
+import org.apache.parquet.hadoop.metadata.ColumnPath;
+import org.apache.parquet.internal.column.columnindex.ColumnIndex;
+import org.apache.parquet.internal.column.columnindex.OffsetIndex;
+import org.apache.parquet.internal.filter2.columnindex.ColumnIndexStore;
+import org.apache.parquet.internal.hadoop.metadata.IndexReference;
+import org.apache.parquet.io.SeekableInputStream;
+
+class ColumnIndexReader implements ColumnIndexStore {
+ private static final Logger LOG = LoggerFactory.getLogger(ColumnIndexReader.class);
+
+ // Used for columns are not in this parquet file
+ private static final IndexStore MISSING_INDEX_STORE =
+ new IndexStore() {
+ @Override
+ public ColumnIndex getColumnIndex() {
+ return null;
+ }
+
+ @Override
+ public OffsetIndex getOffsetIndex() {
+ return null;
+ }
+ };
+
+ private static final ColumnIndexReader EMPTY =
+ new ColumnIndexReader(new BlockMetaData(), Collections.emptySet(), null, null) {
+ @Override
+ public ColumnIndex getColumnIndex(ColumnPath column) {
+ return null;
+ }
+
+ @Override
+ public OffsetIndex getOffsetIndex(ColumnPath column) {
+ throw new MissingOffsetIndexException(column);
+ }
+ };
+
+ private final InternalFileDecryptor fileDecryptor;
+ private final SeekableInputStream inputStream;
+ private final Map<ColumnPath, IndexStore> store;
+
+ /**
+ * Creates a column index store which lazily reads column/offset indexes for the columns in paths.
+ * Paths are the set of columns used for the projection.
+ */
+ static ColumnIndexReader create(
+ BlockMetaData block,
+ Set<ColumnPath> paths,
+ InternalFileDecryptor fileDecryptor,
+ SeekableInputStream inputStream) {
+ try {
+ return new ColumnIndexReader(block, paths, fileDecryptor, inputStream);
+ } catch (MissingOffsetIndexException e) {
+ return EMPTY;
+ }
+ }
+
+ private ColumnIndexReader(
+ BlockMetaData block,
+ Set<ColumnPath> paths,
+ InternalFileDecryptor fileDecryptor,
+ SeekableInputStream inputStream) {
+ this.fileDecryptor = fileDecryptor;
+ this.inputStream = inputStream;
+ Map<ColumnPath, IndexStore> store = new HashMap<>();
+ for (ColumnChunkMetaData column : block.getColumns()) {
+ ColumnPath path = column.getPath();
+ if (paths.contains(path)) {
+ store.put(path, new IndexStoreImpl(column));
+ }
+ }
+ this.store = store;
+ }
+
+ @Override
+ public ColumnIndex getColumnIndex(ColumnPath column) {
+ return store.getOrDefault(column, MISSING_INDEX_STORE).getColumnIndex();
+ }
+
+ @Override
+ public OffsetIndex getOffsetIndex(ColumnPath column) {
+ return store.getOrDefault(column, MISSING_INDEX_STORE).getOffsetIndex();
+ }
+
+ private interface IndexStore {
+ ColumnIndex getColumnIndex();
+
+ OffsetIndex getOffsetIndex();
+ }
+
+ private class IndexStoreImpl implements IndexStore {
+ private final ColumnChunkMetaData meta;
+ private ColumnIndex columnIndex;
+ private boolean columnIndexRead;
+ private final OffsetIndex offsetIndex;
+
+ IndexStoreImpl(ColumnChunkMetaData meta) {
+ this.meta = meta;
+ OffsetIndex oi;
+ try {
+ oi = readOffsetIndex(meta);
+ } catch (IOException e) {
+ // If the I/O issue still stands it will fail the reading later;
+ // otherwise we fail the filtering only with a missing offset index.
+ LOG.warn("Unable to read offset index for column {}", meta.getPath(), e);
+ oi = null;
+ }
+ if (oi == null) {
+ throw new MissingOffsetIndexException(meta.getPath());
+ }
+ offsetIndex = oi;
+ }
+
+ @Override
+ public ColumnIndex getColumnIndex() {
+ if (!columnIndexRead) {
+ try {
+ columnIndex = readColumnIndex(meta);
+ } catch (IOException e) {
+ // If the I/O issue still stands it will fail the reading later;
+ // otherwise we fail the filtering only with a missing column index.
+ LOG.warn("Unable to read column index for column {}", meta.getPath(), e);
+ }
+ columnIndexRead = true;
+ }
+ return columnIndex;
+ }
+
+ @Override
+ public OffsetIndex getOffsetIndex() {
+ return offsetIndex;
+ }
+ }
+
+ // Visible for testing
+ ColumnIndex readColumnIndex(ColumnChunkMetaData column) throws IOException {
+ IndexReference ref = column.getColumnIndexReference();
+ if (ref == null) {
+ return null;
+ }
+ inputStream.seek(ref.getOffset());
+
+ BlockCipher.Decryptor columnIndexDecryptor = null;
+ byte[] columnIndexAAD = null;
+ if (null != fileDecryptor && !fileDecryptor.plaintextFile()) {
+ InternalColumnDecryptionSetup columnDecryptionSetup =
+ fileDecryptor.getColumnSetup(column.getPath());
+ if (columnDecryptionSetup.isEncrypted()) {
+ columnIndexDecryptor = columnDecryptionSetup.getMetaDataDecryptor();
+ columnIndexAAD =
+ AesCipher.createModuleAAD(
+ fileDecryptor.getFileAAD(),
+ ModuleCipherFactory.ModuleType.ColumnIndex,
+ column.getRowGroupOrdinal(),
+ columnDecryptionSetup.getOrdinal(),
+ -1);
+ }
+ }
+ return ParquetMetadataConverter.fromParquetColumnIndex(
+ column.getPrimitiveType(),
+ Util.readColumnIndex(inputStream, columnIndexDecryptor, columnIndexAAD));
+ }
+
+ // Visible for testing
Review Comment:
Can this comment be replaced with an annotation?
##########
common/src/main/java/org/apache/comet/NativeBase.java:
##########
@@ -0,0 +1,278 @@
+/*
+ * 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;
+
+import java.io.BufferedReader;
+import java.io.File;
+import java.io.FilenameFilter;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.nio.file.Files;
+import java.nio.file.StandardCopyOption;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.spark.sql.comet.util.Utils;
+
+import static org.apache.comet.Constants.LOG_CONF_NAME;
+import static org.apache.comet.Constants.LOG_CONF_PATH;
+
+/** Base class for JNI bindings. MUST be inherited by all classes that introduce JNI APIs. */
+public abstract class NativeBase {
+ static final String ARROW_UNSAFE_MEMORY_ACCESS = "arrow.enable_unsafe_memory_access";
+ static final String ARROW_NULL_CHECK_FOR_GET = "arrow.enable_null_check_for_get";
+
+ private static final Logger LOG = LoggerFactory.getLogger(NativeBase.class);
+ private static final String NATIVE_LIB_NAME = "comet";
+
+ private static final String libraryToLoad = System.mapLibraryName(NATIVE_LIB_NAME);
+ private static boolean loaded = false;
+ private static final String searchPattern = "libcomet-";
+
+ static {
+ if (!isLoaded()) {
+ load();
+ }
+ }
+
+ public static synchronized boolean isLoaded() {
+ return loaded;
+ }
+
+ // Only for testing
+ static synchronized void setLoaded(boolean b) {
+ loaded = b;
+ }
+
+ static synchronized void load() {
+ if (loaded) {
+ return;
+ }
+
+ cleanupOldTempLibs();
+
+ // Check if the arch used by JDK is the same as arch on the host machine, in particular,
+ // whether x86_64 JDK is used in arm64 Mac
+ if (!checkArch()) {
+ LOG.warn(
+ "Comet is disabled. JDK compiled for x86_64 is used in a Mac based on Apple Silicon. "
+ + "In order to use Comet, Please install a JDK version for ARM64 architecture");
+ return;
+ }
+
+ // Try to load Comet library from the java.library.path.
+ try {
+ System.loadLibrary(libraryToLoad);
+ loaded = true;
+ } catch (UnsatisfiedLinkError ex) {
+ // Doesn't exist, so proceed to loading bundled library.
+ bundleLoadLibrary();
+ }
+
+ initWithLogConf();
+ // Only set the Arrow properties when debugging mode is off
+ if (!(boolean) CometConf.COMET_DEBUG_ENABLED().get()) {
+ setArrowProperties();
+ }
+ }
+
+ /**
+ * Use the bundled native libraries. Functionally equivalent to <code>System.loadLibrary</code>.
+ */
+ private static void bundleLoadLibrary() {
+ String resourceName = resourceName();
+ InputStream is = NativeBase.class.getResourceAsStream(resourceName);
+ if (is == null) {
+ throw new UnsupportedOperationException(
+ "Unsupported OS/arch, cannot find "
+ + resourceName
+ + ". Please try building from source.");
+ }
+
+ File tempLib = null;
+ File tempLibLock = null;
+ try {
+ // Create the .lck file first to avoid a race condition
+ // with other concurrently running Java processes using Comet.
+ tempLibLock = File.createTempFile(searchPattern, "." + os().libExtension + ".lck");
+ tempLib = new File(tempLibLock.getAbsolutePath().replaceFirst(".lck$", ""));
+ // copy to tempLib
+ Files.copy(is, tempLib.toPath(), StandardCopyOption.REPLACE_EXISTING);
+ System.load(tempLib.getAbsolutePath());
+ loaded = true;
+ } catch (IOException e) {
+ throw new IllegalStateException("Cannot unpack libcomet: " + e);
+ } finally {
+ if (!loaded) {
+ if (tempLib != null && tempLib.exists()) {
+ if (!tempLib.delete()) {
+ LOG.error(
+ "Cannot unpack libcomet / cannot delete a temporary native library " + tempLib);
+ }
+ }
+ if (tempLibLock != null && tempLibLock.exists()) {
+ if (!tempLibLock.delete()) {
+ LOG.error(
+ "Cannot unpack libcomet / cannot delete a temporary lock file " + tempLibLock);
+ }
+ }
+ } else {
+ tempLib.deleteOnExit();
+ tempLibLock.deleteOnExit();
+ }
+ }
+ }
+
+ private static void initWithLogConf() {
+ String logConfPath = System.getProperty(LOG_CONF_PATH(), Utils.getConfPath(LOG_CONF_NAME()));
+
+ // If both the system property and the environmental variable failed to find a log
+ // configuration, then fall back to using the deployed default
+ if (logConfPath == null) {
+ LOG.info(
+ "Couldn't locate log file from either COMET_CONF_DIR or comet.log.file.path. "
+ + "Using default log configuration which emits to stdout");
+ logConfPath = "";
+ } else {
+ LOG.info("Using {} for native library logging", logConfPath);
+ }
+ init(logConfPath);
+ }
+
+ private static void cleanupOldTempLibs() {
+ String tempFolder = new File(System.getProperty("java.io.tmpdir")).getAbsolutePath();
+ File dir = new File(tempFolder);
+
+ File[] tempLibFiles =
+ dir.listFiles(
+ new FilenameFilter() {
+ public boolean accept(File dir, String name) {
+ return name.startsWith(searchPattern) && !name.endsWith(".lck");
+ }
+ });
+
+ if (tempLibFiles != null) {
+ for (File tempLibFile : tempLibFiles) {
+ File lckFile = new File(tempLibFile.getAbsolutePath() + ".lck");
+ if (!lckFile.exists()) {
+ try {
+ tempLibFile.delete();
+ } catch (SecurityException e) {
+ LOG.error("Failed to delete old temp lib", e);
+ }
+ }
+ }
+ }
+ }
+
+ // Set Arrow related properties upon initializing native, such as enabling unsafe memory access
+ // as well as disabling null check for get, for performance reasons.
+ private static void setArrowProperties() {
+ setPropertyIfNull(ARROW_UNSAFE_MEMORY_ACCESS, "true");
+ setPropertyIfNull(ARROW_NULL_CHECK_FOR_GET, "false");
+ }
+
+ private static void setPropertyIfNull(String key, String value) {
+ if (System.getProperty(key) == null) {
+ LOG.info("Setting system property {} to {}", key, value);
+ System.setProperty(key, value);
+ } else {
+ LOG.info(
+ "Skip setting system property {} to {}, because it is already set to {}",
Review Comment:
Skipped
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#issuecomment-1930995101
I have started the IP clearance vote: https://lists.apache.org/thread/lj3j4q7snpzrfo3mh3cph26mdpr2jrfx
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#issuecomment-1937567374
Nice work -- so excited!
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#issuecomment-1912497178
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).
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "liurenjie1024 (via GitHub)" <gi...@apache.org>.
liurenjie1024 commented on PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#issuecomment-1912900737
> 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!
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#issuecomment-1925445318
> 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`
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#issuecomment-1918723588
> 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.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#discussion_r1484914849
##########
common/src/main/java/org/apache/comet/parquet/ColumnIndexReader.java:
##########
@@ -0,0 +1,230 @@
+/*
+ * 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.parquet;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Set;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.parquet.crypto.AesCipher;
+import org.apache.parquet.crypto.InternalColumnDecryptionSetup;
+import org.apache.parquet.crypto.InternalFileDecryptor;
+import org.apache.parquet.crypto.ModuleCipherFactory;
+import org.apache.parquet.format.BlockCipher;
+import org.apache.parquet.format.Util;
+import org.apache.parquet.format.converter.ParquetMetadataConverter;
+import org.apache.parquet.hadoop.metadata.BlockMetaData;
+import org.apache.parquet.hadoop.metadata.ColumnChunkMetaData;
+import org.apache.parquet.hadoop.metadata.ColumnPath;
+import org.apache.parquet.internal.column.columnindex.ColumnIndex;
+import org.apache.parquet.internal.column.columnindex.OffsetIndex;
+import org.apache.parquet.internal.filter2.columnindex.ColumnIndexStore;
+import org.apache.parquet.internal.hadoop.metadata.IndexReference;
+import org.apache.parquet.io.SeekableInputStream;
+
+class ColumnIndexReader implements ColumnIndexStore {
+ private static final Logger LOG = LoggerFactory.getLogger(ColumnIndexReader.class);
+
+ // Used for columns are not in this parquet file
+ private static final IndexStore MISSING_INDEX_STORE =
+ new IndexStore() {
+ @Override
+ public ColumnIndex getColumnIndex() {
+ return null;
+ }
+
+ @Override
+ public OffsetIndex getOffsetIndex() {
+ return null;
+ }
+ };
+
+ private static final ColumnIndexReader EMPTY =
+ new ColumnIndexReader(new BlockMetaData(), Collections.emptySet(), null, null) {
+ @Override
+ public ColumnIndex getColumnIndex(ColumnPath column) {
+ return null;
+ }
+
+ @Override
+ public OffsetIndex getOffsetIndex(ColumnPath column) {
+ throw new MissingOffsetIndexException(column);
+ }
+ };
+
+ private final InternalFileDecryptor fileDecryptor;
+ private final SeekableInputStream inputStream;
+ private final Map<ColumnPath, IndexStore> store;
+
+ /**
+ * Creates a column index store which lazily reads column/offset indexes for the columns in paths.
+ * Paths are the set of columns used for the projection.
+ */
+ static ColumnIndexReader create(
+ BlockMetaData block,
+ Set<ColumnPath> paths,
+ InternalFileDecryptor fileDecryptor,
+ SeekableInputStream inputStream) {
+ try {
+ return new ColumnIndexReader(block, paths, fileDecryptor, inputStream);
+ } catch (MissingOffsetIndexException e) {
+ return EMPTY;
+ }
+ }
+
+ private ColumnIndexReader(
+ BlockMetaData block,
+ Set<ColumnPath> paths,
+ InternalFileDecryptor fileDecryptor,
+ SeekableInputStream inputStream) {
+ this.fileDecryptor = fileDecryptor;
+ this.inputStream = inputStream;
+ Map<ColumnPath, IndexStore> store = new HashMap<>();
+ for (ColumnChunkMetaData column : block.getColumns()) {
+ ColumnPath path = column.getPath();
+ if (paths.contains(path)) {
+ store.put(path, new IndexStoreImpl(column));
+ }
+ }
+ this.store = store;
+ }
+
+ @Override
+ public ColumnIndex getColumnIndex(ColumnPath column) {
+ return store.getOrDefault(column, MISSING_INDEX_STORE).getColumnIndex();
+ }
+
+ @Override
+ public OffsetIndex getOffsetIndex(ColumnPath column) {
+ return store.getOrDefault(column, MISSING_INDEX_STORE).getOffsetIndex();
+ }
+
+ private interface IndexStore {
+ ColumnIndex getColumnIndex();
+
+ OffsetIndex getOffsetIndex();
+ }
+
+ private class IndexStoreImpl implements IndexStore {
+ private final ColumnChunkMetaData meta;
+ private ColumnIndex columnIndex;
+ private boolean columnIndexRead;
+ private final OffsetIndex offsetIndex;
+
+ IndexStoreImpl(ColumnChunkMetaData meta) {
+ this.meta = meta;
+ OffsetIndex oi;
+ try {
+ oi = readOffsetIndex(meta);
+ } catch (IOException e) {
+ // If the I/O issue still stands it will fail the reading later;
+ // otherwise we fail the filtering only with a missing offset index.
+ LOG.warn("Unable to read offset index for column {}", meta.getPath(), e);
+ oi = null;
+ }
+ if (oi == null) {
+ throw new MissingOffsetIndexException(meta.getPath());
+ }
+ offsetIndex = oi;
+ }
+
+ @Override
+ public ColumnIndex getColumnIndex() {
+ if (!columnIndexRead) {
+ try {
+ columnIndex = readColumnIndex(meta);
+ } catch (IOException e) {
+ // If the I/O issue still stands it will fail the reading later;
+ // otherwise we fail the filtering only with a missing column index.
+ LOG.warn("Unable to read column index for column {}", meta.getPath(), e);
+ }
+ columnIndexRead = true;
+ }
+ return columnIndex;
+ }
+
+ @Override
+ public OffsetIndex getOffsetIndex() {
+ return offsetIndex;
+ }
+ }
+
+ // Visible for testing
+ ColumnIndex readColumnIndex(ColumnChunkMetaData column) throws IOException {
+ IndexReference ref = column.getColumnIndexReference();
+ if (ref == null) {
+ return null;
+ }
+ inputStream.seek(ref.getOffset());
+
+ BlockCipher.Decryptor columnIndexDecryptor = null;
+ byte[] columnIndexAAD = null;
+ if (null != fileDecryptor && !fileDecryptor.plaintextFile()) {
+ InternalColumnDecryptionSetup columnDecryptionSetup =
+ fileDecryptor.getColumnSetup(column.getPath());
+ if (columnDecryptionSetup.isEncrypted()) {
+ columnIndexDecryptor = columnDecryptionSetup.getMetaDataDecryptor();
+ columnIndexAAD =
+ AesCipher.createModuleAAD(
+ fileDecryptor.getFileAAD(),
+ ModuleCipherFactory.ModuleType.ColumnIndex,
+ column.getRowGroupOrdinal(),
+ columnDecryptionSetup.getOrdinal(),
+ -1);
+ }
+ }
+ return ParquetMetadataConverter.fromParquetColumnIndex(
+ column.getPrimitiveType(),
+ Util.readColumnIndex(inputStream, columnIndexDecryptor, columnIndexAAD));
+ }
+
+ // Visible for testing
Review Comment:
We need to check. I remember we did this explicitly to avoid some additional dependency.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#discussion_r1466735019
##########
common/src/main/java/org/apache/comet/parquet/AbstractColumnReader.java:
##########
@@ -0,0 +1,116 @@
+/*
Review Comment:
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
##########
core/src/execution/mod.rs:
##########
@@ -0,0 +1,55 @@
+// 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.
+
+//! PoC of vectorization execution through JNI to Rust.
+pub mod datafusion;
+pub mod jni_api;
+
+pub mod kernels; // for benchmarking
+
+mod metrics;
+pub mod operators;
+pub mod serde;
+mod timezone;
+pub(crate) mod utils;
+
+// Include generated modules from .proto files.
+#[allow(missing_docs)]
+pub mod spark_expression {
+ include!(concat!("generated", "/spark.spark_expression.rs"));
+}
+
+// Include generated modules from .proto files.
+#[allow(missing_docs)]
+pub mod spark_partitioning {
+ include!(concat!("generated", "/spark.spark_partitioning.rs"));
+}
+
+// Include generated modules from .proto files.
+#[allow(missing_docs)]
+pub mod spark_operator {
+ include!(concat!("generated", "/spark.spark_operator.rs"));
+}
+
+#[cfg(test)]
+mod tests {
Review Comment:
😆
##########
common/src/main/java/org/apache/comet/parquet/BloomFilterReader.java:
##########
@@ -0,0 +1,253 @@
+/*
+ * 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.parquet;
+
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.parquet.column.values.bloomfilter.BlockSplitBloomFilter;
+import org.apache.parquet.column.values.bloomfilter.BloomFilter;
+import org.apache.parquet.crypto.AesCipher;
+import org.apache.parquet.crypto.InternalColumnDecryptionSetup;
+import org.apache.parquet.crypto.InternalFileDecryptor;
+import org.apache.parquet.crypto.ModuleCipherFactory;
+import org.apache.parquet.crypto.ParquetCryptoRuntimeException;
+import org.apache.parquet.filter2.predicate.FilterPredicate;
+import org.apache.parquet.filter2.predicate.Operators;
+import org.apache.parquet.filter2.predicate.UserDefinedPredicate;
+import org.apache.parquet.format.BlockCipher;
+import org.apache.parquet.format.BloomFilterHeader;
+import org.apache.parquet.format.Util;
+import org.apache.parquet.hadoop.metadata.BlockMetaData;
+import org.apache.parquet.hadoop.metadata.ColumnChunkMetaData;
+import org.apache.parquet.hadoop.metadata.ColumnPath;
+import org.apache.parquet.io.SeekableInputStream;
+
+public class BloomFilterReader implements FilterPredicate.Visitor<Boolean> {
Review Comment:
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
##########
core/src/execution/kernels/temporal.rs:
##########
@@ -0,0 +1,438 @@
+// 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.
+
+//! temporal kernels
+
+use chrono::{DateTime, Datelike, Duration, NaiveDateTime, Timelike, Utc};
+
+use std::sync::Arc;
+
+use arrow::{array::*, datatypes::DataType};
+use arrow_array::{
+ downcast_dictionary_array, downcast_temporal_array,
+ temporal_conversions::*,
+ timezone::Tz,
+ types::{ArrowTemporalType, Date32Type, TimestampMicrosecondType},
+ ArrowNumericType,
+};
+
+use arrow_schema::TimeUnit;
+
+use crate::errors::ExpressionError;
+
+// Copied from arrow_arith/temporal.rs
+macro_rules! return_compute_error_with {
+ ($msg:expr, $param:expr) => {
+ return {
+ Err(ExpressionError::ArrowError(format!(
+ "{}: {:?}",
+ $msg, $param
+ )))
+ }
+ };
+}
+
+// The number of days between the beginning of the proleptic gregorian calendar (0001-01-01)
+// and the beginning of the Unix Epoch (1970-01-01)
+const DAYS_TO_UNIX_EPOCH: i32 = 719_163;
+const MICROS_TO_UNIX_EPOCH: i64 = 62_167_132_800 * 1_000_000;
+
+// Copied from arrow_arith/temporal.rs with modification to the output datatype
+// Transforms a array of NaiveDate to an array of Date32 after applying an operation
+fn as_datetime_with_op<A: ArrayAccessor<Item = T::Native>, T: ArrowTemporalType, F>(
+ iter: ArrayIter<A>,
+ mut builder: PrimitiveBuilder<Date32Type>,
+ op: F,
+) -> Date32Array
+where
+ F: Fn(NaiveDateTime) -> i32,
+ i64: From<T::Native>,
+{
+ iter.into_iter().for_each(|value| {
+ if let Some(value) = value {
+ match as_datetime::<T>(i64::from(value)) {
+ Some(dt) => builder.append_value(op(dt)),
+ None => builder.append_null(),
+ }
+ } else {
+ builder.append_null();
+ }
+ });
+
+ builder.finish()
+}
+
+// Based on arrow_arith/temporal.rs:extract_component_from_datetime_array
+// Transforms an array of DateTime<Tz> to an arrayOf TimeStampMicrosecond after applying an
+// operation
+fn as_timestamp_tz_with_op<A: ArrayAccessor<Item = T::Native>, T: ArrowTemporalType, F>(
+ iter: ArrayIter<A>,
+ mut builder: PrimitiveBuilder<TimestampMicrosecondType>,
+ tz: &str,
+ op: F,
+) -> Result<TimestampMicrosecondArray, ExpressionError>
+where
+ F: Fn(DateTime<Tz>) -> i64,
+ i64: From<T::Native>,
+{
+ let tz: Tz = tz.parse()?;
+ for value in iter {
+ match value {
+ Some(value) => match as_datetime_with_timezone::<T>(value.into(), tz) {
+ Some(time) => builder.append_value(op(time)),
+ _ => {
+ return Err(ExpressionError::ArrowError(
+ "Unable to read value as datetime".to_string(),
+ ));
+ }
+ },
+ None => builder.append_null(),
+ }
+ }
+ Ok(builder.finish())
+}
+
+#[inline]
+fn as_days_from_unix_epoch(dt: Option<NaiveDateTime>) -> i32 {
+ dt.unwrap().num_days_from_ce() - DAYS_TO_UNIX_EPOCH
+}
+
+// Apply the Tz to the Naive Date Time,,convert to UTC, and return as microseconds in Unix epoch
+#[inline]
+fn as_micros_from_unix_epoch_utc(dt: Option<DateTime<Tz>>) -> i64 {
+ dt.unwrap().with_timezone(&Utc).timestamp_micros()
+}
+
+#[inline]
+fn trunc_date_to_year<T: Datelike + Timelike>(dt: T) -> Option<T> {
+ Some(dt)
+ .and_then(|d| d.with_nanosecond(0))
+ .and_then(|d| d.with_second(0))
+ .and_then(|d| d.with_minute(0))
+ .and_then(|d| d.with_hour(0))
+ .and_then(|d| d.with_day0(0))
+ .and_then(|d| d.with_month0(0))
+}
+
+/// returns the month of the beginning of the quarter
+#[inline]
+fn quarter_month<T: Datelike>(dt: &T) -> u32 {
+ 1 + 3 * ((dt.month() - 1) / 3)
+}
+
+#[inline]
+fn trunc_date_to_quarter<T: Datelike + Timelike>(dt: T) -> Option<T> {
+ Some(dt)
+ .and_then(|d| d.with_nanosecond(0))
+ .and_then(|d| d.with_second(0))
+ .and_then(|d| d.with_minute(0))
+ .and_then(|d| d.with_hour(0))
+ .and_then(|d| d.with_day0(0))
+ .and_then(|d| d.with_month(quarter_month(&d)))
+}
+
+#[inline]
+fn trunc_date_to_month<T: Datelike + Timelike>(dt: T) -> Option<T> {
+ Some(dt)
+ .and_then(|d| d.with_nanosecond(0))
+ .and_then(|d| d.with_second(0))
+ .and_then(|d| d.with_minute(0))
+ .and_then(|d| d.with_hour(0))
+ .and_then(|d| d.with_day0(0))
+}
+
+#[inline]
+fn trunc_date_to_week<T>(dt: T) -> Option<T>
+where
+ T: Datelike + Timelike + std::ops::Sub<Duration, Output = T> + Copy,
+{
+ Some(dt)
+ .map(|d| d - Duration::seconds(60 * 60 * 24 * d.weekday() as i64))
+ .and_then(|d| d.with_nanosecond(0))
+ .and_then(|d| d.with_second(0))
+ .and_then(|d| d.with_minute(0))
+ .and_then(|d| d.with_hour(0))
+}
+
+#[inline]
+fn trunc_date_to_day<T: Timelike>(dt: T) -> Option<T> {
+ Some(dt)
+ .and_then(|d| d.with_nanosecond(0))
+ .and_then(|d| d.with_second(0))
+ .and_then(|d| d.with_minute(0))
+ .and_then(|d| d.with_hour(0))
+}
+
+#[inline]
+fn trunc_date_to_hour<T: Timelike>(dt: T) -> Option<T> {
+ Some(dt)
+ .and_then(|d| d.with_nanosecond(0))
+ .and_then(|d| d.with_second(0))
+ .and_then(|d| d.with_minute(0))
+}
+
+#[inline]
+fn trunc_date_to_minute<T: Timelike>(dt: T) -> Option<T> {
+ Some(dt)
+ .and_then(|d| d.with_nanosecond(0))
+ .and_then(|d| d.with_second(0))
+}
+
+#[inline]
+fn trunc_date_to_second<T: Timelike>(dt: T) -> Option<T> {
+ Some(dt).and_then(|d| d.with_nanosecond(0))
+}
+
+#[inline]
+fn trunc_date_to_ms<T: Timelike>(dt: T) -> Option<T> {
+ Some(dt).and_then(|d| d.with_nanosecond(1_000_000 * (d.nanosecond() / 1_000_000)))
+}
+
+#[inline]
+fn trunc_date_to_microsec<T: Timelike>(dt: T) -> Option<T> {
+ 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> {
Review Comment:
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
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#discussion_r1465707245
##########
rust-toolchain:
##########
@@ -0,0 +1 @@
+nightly-2023-09-05
Review Comment:
I'm curious to know why nightly Rust is required. It would be good to add some docs on this at some point.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#issuecomment-1908863763
Thank you @sunchao -- I plan to give this a review over the next day or two
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#discussion_r1468086461
##########
core/src/execution/kernels/temporal.rs:
##########
@@ -0,0 +1,438 @@
+// 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.
+
+//! temporal kernels
+
+use chrono::{DateTime, Datelike, Duration, NaiveDateTime, Timelike, Utc};
+
+use std::sync::Arc;
+
+use arrow::{array::*, datatypes::DataType};
+use arrow_array::{
+ downcast_dictionary_array, downcast_temporal_array,
+ temporal_conversions::*,
+ timezone::Tz,
+ types::{ArrowTemporalType, Date32Type, TimestampMicrosecondType},
+ ArrowNumericType,
+};
+
+use arrow_schema::TimeUnit;
+
+use crate::errors::ExpressionError;
+
+// Copied from arrow_arith/temporal.rs
+macro_rules! return_compute_error_with {
+ ($msg:expr, $param:expr) => {
+ return {
+ Err(ExpressionError::ArrowError(format!(
+ "{}: {:?}",
+ $msg, $param
+ )))
+ }
+ };
+}
+
+// The number of days between the beginning of the proleptic gregorian calendar (0001-01-01)
+// and the beginning of the Unix Epoch (1970-01-01)
+const DAYS_TO_UNIX_EPOCH: i32 = 719_163;
+const MICROS_TO_UNIX_EPOCH: i64 = 62_167_132_800 * 1_000_000;
+
+// Copied from arrow_arith/temporal.rs with modification to the output datatype
+// Transforms a array of NaiveDate to an array of Date32 after applying an operation
+fn as_datetime_with_op<A: ArrayAccessor<Item = T::Native>, T: ArrowTemporalType, F>(
+ iter: ArrayIter<A>,
+ mut builder: PrimitiveBuilder<Date32Type>,
+ op: F,
+) -> Date32Array
+where
+ F: Fn(NaiveDateTime) -> i32,
+ i64: From<T::Native>,
+{
+ iter.into_iter().for_each(|value| {
+ if let Some(value) = value {
+ match as_datetime::<T>(i64::from(value)) {
+ Some(dt) => builder.append_value(op(dt)),
+ None => builder.append_null(),
+ }
+ } else {
+ builder.append_null();
+ }
+ });
+
+ builder.finish()
+}
+
+// Based on arrow_arith/temporal.rs:extract_component_from_datetime_array
+// Transforms an array of DateTime<Tz> to an arrayOf TimeStampMicrosecond after applying an
+// operation
+fn as_timestamp_tz_with_op<A: ArrayAccessor<Item = T::Native>, T: ArrowTemporalType, F>(
+ iter: ArrayIter<A>,
+ mut builder: PrimitiveBuilder<TimestampMicrosecondType>,
+ tz: &str,
+ op: F,
+) -> Result<TimestampMicrosecondArray, ExpressionError>
+where
+ F: Fn(DateTime<Tz>) -> i64,
+ i64: From<T::Native>,
+{
+ let tz: Tz = tz.parse()?;
+ for value in iter {
+ match value {
+ Some(value) => match as_datetime_with_timezone::<T>(value.into(), tz) {
+ Some(time) => builder.append_value(op(time)),
+ _ => {
+ return Err(ExpressionError::ArrowError(
+ "Unable to read value as datetime".to_string(),
+ ));
+ }
+ },
+ None => builder.append_null(),
+ }
+ }
+ Ok(builder.finish())
+}
+
+#[inline]
+fn as_days_from_unix_epoch(dt: Option<NaiveDateTime>) -> i32 {
+ dt.unwrap().num_days_from_ce() - DAYS_TO_UNIX_EPOCH
+}
+
+// Apply the Tz to the Naive Date Time,,convert to UTC, and return as microseconds in Unix epoch
+#[inline]
+fn as_micros_from_unix_epoch_utc(dt: Option<DateTime<Tz>>) -> i64 {
+ dt.unwrap().with_timezone(&Utc).timestamp_micros()
+}
+
+#[inline]
+fn trunc_date_to_year<T: Datelike + Timelike>(dt: T) -> Option<T> {
+ Some(dt)
+ .and_then(|d| d.with_nanosecond(0))
+ .and_then(|d| d.with_second(0))
+ .and_then(|d| d.with_minute(0))
+ .and_then(|d| d.with_hour(0))
+ .and_then(|d| d.with_day0(0))
+ .and_then(|d| d.with_month0(0))
+}
+
+/// returns the month of the beginning of the quarter
+#[inline]
+fn quarter_month<T: Datelike>(dt: &T) -> u32 {
+ 1 + 3 * ((dt.month() - 1) / 3)
+}
+
+#[inline]
+fn trunc_date_to_quarter<T: Datelike + Timelike>(dt: T) -> Option<T> {
+ Some(dt)
+ .and_then(|d| d.with_nanosecond(0))
+ .and_then(|d| d.with_second(0))
+ .and_then(|d| d.with_minute(0))
+ .and_then(|d| d.with_hour(0))
+ .and_then(|d| d.with_day0(0))
+ .and_then(|d| d.with_month(quarter_month(&d)))
+}
+
+#[inline]
+fn trunc_date_to_month<T: Datelike + Timelike>(dt: T) -> Option<T> {
+ Some(dt)
+ .and_then(|d| d.with_nanosecond(0))
+ .and_then(|d| d.with_second(0))
+ .and_then(|d| d.with_minute(0))
+ .and_then(|d| d.with_hour(0))
+ .and_then(|d| d.with_day0(0))
+}
+
+#[inline]
+fn trunc_date_to_week<T>(dt: T) -> Option<T>
+where
+ T: Datelike + Timelike + std::ops::Sub<Duration, Output = T> + Copy,
+{
+ Some(dt)
+ .map(|d| d - Duration::seconds(60 * 60 * 24 * d.weekday() as i64))
+ .and_then(|d| d.with_nanosecond(0))
+ .and_then(|d| d.with_second(0))
+ .and_then(|d| d.with_minute(0))
+ .and_then(|d| d.with_hour(0))
+}
+
+#[inline]
+fn trunc_date_to_day<T: Timelike>(dt: T) -> Option<T> {
+ Some(dt)
+ .and_then(|d| d.with_nanosecond(0))
+ .and_then(|d| d.with_second(0))
+ .and_then(|d| d.with_minute(0))
+ .and_then(|d| d.with_hour(0))
+}
+
+#[inline]
+fn trunc_date_to_hour<T: Timelike>(dt: T) -> Option<T> {
+ Some(dt)
+ .and_then(|d| d.with_nanosecond(0))
+ .and_then(|d| d.with_second(0))
+ .and_then(|d| d.with_minute(0))
+}
+
+#[inline]
+fn trunc_date_to_minute<T: Timelike>(dt: T) -> Option<T> {
+ Some(dt)
+ .and_then(|d| d.with_nanosecond(0))
+ .and_then(|d| d.with_second(0))
+}
+
+#[inline]
+fn trunc_date_to_second<T: Timelike>(dt: T) -> Option<T> {
+ Some(dt).and_then(|d| d.with_nanosecond(0))
+}
+
+#[inline]
+fn trunc_date_to_ms<T: Timelike>(dt: T) -> Option<T> {
+ Some(dt).and_then(|d| d.with_nanosecond(1_000_000 * (d.nanosecond() / 1_000_000)))
+}
+
+#[inline]
+fn trunc_date_to_microsec<T: Timelike>(dt: T) -> Option<T> {
+ 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> {
Review Comment:
This is what I have in mind, BTW, in case anyone has time to review: https://github.com/apache/arrow-datafusion/pull/8705
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#issuecomment-1913886292
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.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#discussion_r1475315457
##########
README.md:
##########
@@ -1 +1,60 @@
-# arrow-datafusion-comet
+<!--
+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.
+-->
+
+# 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.
Review Comment:
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.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#issuecomment-1925444681
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-s
ys, 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, vers
ion_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?
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#issuecomment-1936753209
Thanks all for the help on this!
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#discussion_r1467051187
##########
core/src/execution/kernels/temporal.rs:
##########
@@ -0,0 +1,438 @@
+// 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.
+
+//! temporal kernels
+
+use chrono::{DateTime, Datelike, Duration, NaiveDateTime, Timelike, Utc};
+
+use std::sync::Arc;
+
+use arrow::{array::*, datatypes::DataType};
+use arrow_array::{
+ downcast_dictionary_array, downcast_temporal_array,
+ temporal_conversions::*,
+ timezone::Tz,
+ types::{ArrowTemporalType, Date32Type, TimestampMicrosecondType},
+ ArrowNumericType,
+};
+
+use arrow_schema::TimeUnit;
+
+use crate::errors::ExpressionError;
+
+// Copied from arrow_arith/temporal.rs
+macro_rules! return_compute_error_with {
+ ($msg:expr, $param:expr) => {
+ return {
+ Err(ExpressionError::ArrowError(format!(
+ "{}: {:?}",
+ $msg, $param
+ )))
+ }
+ };
+}
+
+// The number of days between the beginning of the proleptic gregorian calendar (0001-01-01)
+// and the beginning of the Unix Epoch (1970-01-01)
+const DAYS_TO_UNIX_EPOCH: i32 = 719_163;
+const MICROS_TO_UNIX_EPOCH: i64 = 62_167_132_800 * 1_000_000;
+
+// Copied from arrow_arith/temporal.rs with modification to the output datatype
+// Transforms a array of NaiveDate to an array of Date32 after applying an operation
+fn as_datetime_with_op<A: ArrayAccessor<Item = T::Native>, T: ArrowTemporalType, F>(
+ iter: ArrayIter<A>,
+ mut builder: PrimitiveBuilder<Date32Type>,
+ op: F,
+) -> Date32Array
+where
+ F: Fn(NaiveDateTime) -> i32,
+ i64: From<T::Native>,
+{
+ iter.into_iter().for_each(|value| {
+ if let Some(value) = value {
+ match as_datetime::<T>(i64::from(value)) {
+ Some(dt) => builder.append_value(op(dt)),
+ None => builder.append_null(),
+ }
+ } else {
+ builder.append_null();
+ }
+ });
+
+ builder.finish()
+}
+
+// Based on arrow_arith/temporal.rs:extract_component_from_datetime_array
+// Transforms an array of DateTime<Tz> to an arrayOf TimeStampMicrosecond after applying an
+// operation
+fn as_timestamp_tz_with_op<A: ArrayAccessor<Item = T::Native>, T: ArrowTemporalType, F>(
+ iter: ArrayIter<A>,
+ mut builder: PrimitiveBuilder<TimestampMicrosecondType>,
+ tz: &str,
+ op: F,
+) -> Result<TimestampMicrosecondArray, ExpressionError>
+where
+ F: Fn(DateTime<Tz>) -> i64,
+ i64: From<T::Native>,
+{
+ let tz: Tz = tz.parse()?;
+ for value in iter {
+ match value {
+ Some(value) => match as_datetime_with_timezone::<T>(value.into(), tz) {
+ Some(time) => builder.append_value(op(time)),
+ _ => {
+ return Err(ExpressionError::ArrowError(
+ "Unable to read value as datetime".to_string(),
+ ));
+ }
+ },
+ None => builder.append_null(),
+ }
+ }
+ Ok(builder.finish())
+}
+
+#[inline]
+fn as_days_from_unix_epoch(dt: Option<NaiveDateTime>) -> i32 {
+ dt.unwrap().num_days_from_ce() - DAYS_TO_UNIX_EPOCH
+}
+
+// Apply the Tz to the Naive Date Time,,convert to UTC, and return as microseconds in Unix epoch
+#[inline]
+fn as_micros_from_unix_epoch_utc(dt: Option<DateTime<Tz>>) -> i64 {
+ dt.unwrap().with_timezone(&Utc).timestamp_micros()
+}
+
+#[inline]
+fn trunc_date_to_year<T: Datelike + Timelike>(dt: T) -> Option<T> {
+ Some(dt)
+ .and_then(|d| d.with_nanosecond(0))
+ .and_then(|d| d.with_second(0))
+ .and_then(|d| d.with_minute(0))
+ .and_then(|d| d.with_hour(0))
+ .and_then(|d| d.with_day0(0))
+ .and_then(|d| d.with_month0(0))
+}
+
+/// returns the month of the beginning of the quarter
+#[inline]
+fn quarter_month<T: Datelike>(dt: &T) -> u32 {
+ 1 + 3 * ((dt.month() - 1) / 3)
+}
+
+#[inline]
+fn trunc_date_to_quarter<T: Datelike + Timelike>(dt: T) -> Option<T> {
+ Some(dt)
+ .and_then(|d| d.with_nanosecond(0))
+ .and_then(|d| d.with_second(0))
+ .and_then(|d| d.with_minute(0))
+ .and_then(|d| d.with_hour(0))
+ .and_then(|d| d.with_day0(0))
+ .and_then(|d| d.with_month(quarter_month(&d)))
+}
+
+#[inline]
+fn trunc_date_to_month<T: Datelike + Timelike>(dt: T) -> Option<T> {
+ Some(dt)
+ .and_then(|d| d.with_nanosecond(0))
+ .and_then(|d| d.with_second(0))
+ .and_then(|d| d.with_minute(0))
+ .and_then(|d| d.with_hour(0))
+ .and_then(|d| d.with_day0(0))
+}
+
+#[inline]
+fn trunc_date_to_week<T>(dt: T) -> Option<T>
+where
+ T: Datelike + Timelike + std::ops::Sub<Duration, Output = T> + Copy,
+{
+ Some(dt)
+ .map(|d| d - Duration::seconds(60 * 60 * 24 * d.weekday() as i64))
+ .and_then(|d| d.with_nanosecond(0))
+ .and_then(|d| d.with_second(0))
+ .and_then(|d| d.with_minute(0))
+ .and_then(|d| d.with_hour(0))
+}
+
+#[inline]
+fn trunc_date_to_day<T: Timelike>(dt: T) -> Option<T> {
+ Some(dt)
+ .and_then(|d| d.with_nanosecond(0))
+ .and_then(|d| d.with_second(0))
+ .and_then(|d| d.with_minute(0))
+ .and_then(|d| d.with_hour(0))
+}
+
+#[inline]
+fn trunc_date_to_hour<T: Timelike>(dt: T) -> Option<T> {
+ Some(dt)
+ .and_then(|d| d.with_nanosecond(0))
+ .and_then(|d| d.with_second(0))
+ .and_then(|d| d.with_minute(0))
+}
+
+#[inline]
+fn trunc_date_to_minute<T: Timelike>(dt: T) -> Option<T> {
+ Some(dt)
+ .and_then(|d| d.with_nanosecond(0))
+ .and_then(|d| d.with_second(0))
+}
+
+#[inline]
+fn trunc_date_to_second<T: Timelike>(dt: T) -> Option<T> {
+ Some(dt).and_then(|d| d.with_nanosecond(0))
+}
+
+#[inline]
+fn trunc_date_to_ms<T: Timelike>(dt: T) -> Option<T> {
+ Some(dt).and_then(|d| d.with_nanosecond(1_000_000 * (d.nanosecond() / 1_000_000)))
+}
+
+#[inline]
+fn trunc_date_to_microsec<T: Timelike>(dt: T) -> Option<T> {
+ 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> {
Review Comment:
Definitely!
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#discussion_r1467050828
##########
core/src/execution/mod.rs:
##########
@@ -0,0 +1,55 @@
+// 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.
+
+//! PoC of vectorization execution through JNI to Rust.
+pub mod datafusion;
+pub mod jni_api;
+
+pub mod kernels; // for benchmarking
+
+mod metrics;
+pub mod operators;
+pub mod serde;
+mod timezone;
+pub(crate) mod utils;
+
+// Include generated modules from .proto files.
+#[allow(missing_docs)]
+pub mod spark_expression {
+ include!(concat!("generated", "/spark.spark_expression.rs"));
+}
+
+// Include generated modules from .proto files.
+#[allow(missing_docs)]
+pub mod spark_partitioning {
+ include!(concat!("generated", "/spark.spark_partitioning.rs"));
+}
+
+// Include generated modules from .proto files.
+#[allow(missing_docs)]
+pub mod spark_operator {
+ include!(concat!("generated", "/spark.spark_operator.rs"));
+}
+
+#[cfg(test)]
+mod tests {
Review Comment:
:smirk:
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#issuecomment-1913367405
I manually run the script with 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.
I think core/.lldbinit is committed wrongly. It is not necessary. Removed it.
Only missing license header is Makefile. I just added it.
Except for the script related files (filtered_rat.txt, rat.txt, rat_exclude_files.txt), q1
I don't see any source codes
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#issuecomment-1913417666
Thanks!
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#issuecomment-1913349283
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.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#issuecomment-1925445213
I manually checked the maven dependencies are licenses are all good.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Initial PR [arrow-datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on PR #1:
URL: https://github.com/apache/arrow-datafusion-comet/pull/1#issuecomment-1915346325
@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 columnar shuffle support and unified memory management, before rolling out more features to our customers.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org