You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "eejbyfeldt (via GitHub)" <gi...@apache.org> on 2024/08/07 08:16:16 UTC
[PR] feat: Optimze CreateNamedStruct preserve dictionaries [datafusion-comet]
eejbyfeldt opened a new pull request, #789:
URL: https://github.com/apache/datafusion-comet/pull/789
Instead of serializing the return data_type we just serialize the field
names. The original implmentation was done as it lead to slightly
simpler implementation, but it clear from #750 that this was the wrong
choice and leads to issues with the physical data_type.
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## How are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Optimze CreateNamedStruct preserve dictionaries [datafusion-comet]
Posted by "eejbyfeldt (via GitHub)" <gi...@apache.org>.
eejbyfeldt commented on code in PR #789:
URL: https://github.com/apache/datafusion-comet/pull/789#discussion_r1710103395
##########
native/spark-expr/src/structs.rs:
##########
@@ -136,7 +124,7 @@ impl PartialEq<dyn Any> for CreateNamedStruct {
.iter()
.zip(x.values.iter())
.all(|(a, b)| a.eq(b))
- && self.data_type.eq(&x.data_type)
+ && self.names.iter().zip(x.names.iter()).all(|(a, b)| a.eq(b))
Review Comment:
Good call. Added the checks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Optimze CreateNamedStruct preserve dictionaries [datafusion-comet]
Posted by "eejbyfeldt (via GitHub)" <gi...@apache.org>.
eejbyfeldt commented on PR #789:
URL: https://github.com/apache/datafusion-comet/pull/789#issuecomment-2276498187
I think spark will be "smart" enough to push that filter through the project and peform it before the struct is constructed and therefore this change might not really affect performance. Consider this slightly modified query (since I did not get yours to work as is)
```
scala> spark.sql("SELECT s FROM (SELECT struct(_1, _2) as s FROM tbl) WHERE s._1 RLIKE '^[A-Z]{1}'").explain()
== Physical Plan ==
*(1) Project [struct(_1, _1#10, _2, _2#11) AS s#32]
+- *(1) Filter (isnotnull(_1#10) AND RLIKE(_1#10, ^[A-Z]{1}))
+- *(1) ColumnarToRow
+- FileScan parquet spark_catalog.default.tbl[_1#10,_2#11] Batched: true, DataFilters: [isnotnull(_1#10), RLIKE(_1#10, ^[A-Z]{1})], Format: Parquet, Location: InMemoryFileIndex(1 paths)[file:/home/eejbyfeldt/dev/apache/datafusion-comet/spark-warehouse/tbl], PartitionFilters: [], PushedFilters: [IsNotNull(_1)], ReadSchema: struct<_1:string,_2:int>
```
Notice that the RLIKE is in the filter before the struct is constructed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Optimze CreateNamedStruct preserve dictionaries [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #789:
URL: https://github.com/apache/datafusion-comet/pull/789#discussion_r1707975432
##########
native/spark-expr/src/structs.rs:
##########
@@ -136,7 +124,7 @@ impl PartialEq<dyn Any> for CreateNamedStruct {
.iter()
.zip(x.values.iter())
.all(|(a, b)| a.eq(b))
- && self.data_type.eq(&x.data_type)
+ && self.names.iter().zip(x.names.iter()).all(|(a, b)| a.eq(b))
Review Comment:
do we also need a check that `self.names().len() == x.names().len()`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Optimze CreateNamedStruct preserve dictionaries [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on PR #789:
URL: https://github.com/apache/datafusion-comet/pull/789#issuecomment-2274514165
is it possible to post any kind of benchmarks to show the improvements?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Optimze CreateNamedStruct preserve dictionaries [datafusion-comet]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #789:
URL: https://github.com/apache/datafusion-comet/pull/789#issuecomment-2274472914
## [Codecov](https://app.codecov.io/gh/apache/datafusion-comet/pull/789?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `10.00000%` with `9 lines` in your changes missing coverage. Please review.
> Project coverage is 33.70%. Comparing base [(`c4bd3db`)](https://app.codecov.io/gh/apache/datafusion-comet/commit/c4bd3dbb8099428a5e486f079a040de6e67efded?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`2a55eb5`)](https://app.codecov.io/gh/apache/datafusion-comet/commit/2a55eb537720e9f3558d24746f123b7b463662b9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
| [Files](https://app.codecov.io/gh/apache/datafusion-comet/pull/789?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...n/java/org/apache/comet/vector/CometMapVector.java](https://app.codecov.io/gh/apache/datafusion-comet/pull/789?src=pr&el=tree&filepath=common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fcomet%2Fvector%2FCometMapVector.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9jb21ldC92ZWN0b3IvQ29tZXRNYXBWZWN0b3IuamF2YQ==) | 0.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion-comet/pull/789?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ava/org/apache/comet/vector/CometStructVector.java](https://app.codecov.io/gh/apache/datafusion-comet/pull/789?src=pr&el=tree&filepath=common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fcomet%2Fvector%2FCometStructVector.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9jb21ldC92ZWN0b3IvQ29tZXRTdHJ1Y3RWZWN0b3IuamF2YQ==) | 0.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion-comet/pull/789?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...main/java/org/apache/comet/vector/CometVector.java](https://app.codecov.io/gh/apache/datafusion-comet/pull/789?src=pr&el=tree&filepath=common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fcomet%2Fvector%2FCometVector.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9jb21ldC92ZWN0b3IvQ29tZXRWZWN0b3IuamF2YQ==) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion-comet/pull/789?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../scala/org/apache/comet/serde/QueryPlanSerde.scala](https://app.codecov.io/gh/apache/datafusion-comet/pull/789?src=pr&el=tree&filepath=spark%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Fcomet%2Fserde%2FQueryPlanSerde.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3Bhcmsvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9jb21ldC9zZXJkZS9RdWVyeVBsYW5TZXJkZS5zY2FsYQ==) | 50.00% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/datafusion-comet/pull/789?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #789 +/- ##
============================================
- Coverage 33.74% 33.70% -0.05%
- Complexity 856 869 +13
============================================
Files 112 112
Lines 42730 42781 +51
Branches 9435 9426 -9
============================================
- Hits 14419 14418 -1
- Misses 25331 25390 +59
+ Partials 2980 2973 -7
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/datafusion-comet/pull/789?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Optimze CreateNamedStruct preserve dictionaries [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #789:
URL: https://github.com/apache/datafusion-comet/pull/789
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Optimze CreateNamedStruct preserve dictionaries [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #789:
URL: https://github.com/apache/datafusion-comet/pull/789#issuecomment-2276484812
> > is it possible to post any kind of benchmarks to show the improvements?
>
> I think the expected improvements from this are based on a heuristic that it beneficial to continue to use dictionary encoding whenever possible. So the improvement will depend on what other expressions are used in combination with it. Therefore I think would be possible to construct a benchmark to show almost any or no benefit. Do you have something more specific benchmark in mind or what worry do we want to resolve with it?
I'd like to have a go at proving the benefit of this PR (to help with my own understanding of how dictionary types can affect performance). I am thinking of running something like this:
```sql
SELECT struct(foo, bar) as s FROM tbl WHERE s.foo RLIKE '^[A-Z]{1}'
```
My hypothesis is that this will be faster if we preserve the dictionary type because 1) the regexp can be evaluated on fewer rows, and 2) we avoid the cost of unpacking the dictionary in the first place
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Optimze CreateNamedStruct preserve dictionaries [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #789:
URL: https://github.com/apache/datafusion-comet/pull/789#issuecomment-2278792169
Thanks for the review @kazuyukitanimura
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Optimze CreateNamedStruct preserve dictionaries [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #789:
URL: https://github.com/apache/datafusion-comet/pull/789#issuecomment-2278573348
I've been testing from Spark shell with the following and with some debug logging added to `ScanExec` and `CreateNamedStruct`:
```scala
spark.read.parquet("/mnt/bigdata/tpcds/sf100/item.parquet").createTempView("item")
val df = spark.sql("SELECT struct(i_product_name) as item_detail from item")
spark.time(df.write.format("noop").mode("overwrite").save())
```
When I run this in the main branch, I see:
```
creating ScanStream with schema Schema { fields: [Field { name: "col_0", data_type: Dictionary(Int32, Utf8), nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }], metadata: {} }
ScanStream::build_record_batch() with 8192 rows
CreateNamedStruct unpacking dictionary
ScanStream::build_record_batch() with 8192 rows
CreateNamedStruct unpacking dictionary
...
ScanStream::build_record_batch() with 8192 rows
ScanExec casting from Utf8 to Dictionary(Int32, Utf8)
CreateNamedStruct unpacking dictionary
ScanStream::build_record_batch() with 8192 rows
ScanExec casting from Utf8 to Dictionary(Int32, Utf8)
```
It was interesting to learn that `ScanExec` processes some batches where the data is already dictionary-encoded and some batches where if was not. ScanExec will then convert to dictionary-encoded for consistency. In either case, `CreateNamedStruct` was then unpacking those arrays immediately afterwards.
In the current PR, we are no longer unpacking them when creating the struct, which is avoiding some compute cost and avoiding using more memory.
@kazuyukitanimura I didn't run a microbenchmark to show the time saved, but I think this demonstrates that we are avoiding an operation that is not needed, and would result in some overhead (time and memory)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Optimze CreateNamedStruct preserve dictionaries [datafusion-comet]
Posted by "eejbyfeldt (via GitHub)" <gi...@apache.org>.
eejbyfeldt commented on PR #789:
URL: https://github.com/apache/datafusion-comet/pull/789#issuecomment-2276435837
> is it possible to post any kind of benchmarks to show the improvements?
I think the expected improvements from this are based on a heuristic that it beneficial to continue to use dictionary encoding whenever possible. So the improvement will depend on what other expressions are used in combination with it. Therefore I think would be possible to construct a benchmark to show almost any or no benefit. Do you have something more specific benchmark in mind or what worry do we want to resolve with it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org