datafusion icon indicating copy to clipboard operation
datafusion copied to clipboard

Remove offset if its zero

Open turbo1912 opened this issue 2 years ago • 2 comments

Which issue does this PR close?

Closes #2584

Rationale for this change

If offset is 0, it should be a no-op and can be optimized out.

Questions to the reviewer? Current proposed change adds this optimization to an existing slightly related optimizer. Does this require its own optimizer independent from eliminate_limit?

turbo1912 avatar Aug 11 '22 03:08 turbo1912

Codecov Report

Merging #3102 (c2e0d18) into master (85d5363) will increase coverage by 0.01%. The diff coverage is 94.53%.

:exclamation: Current head c2e0d18 differs from pull request most recent head 67741f8. Consider uploading reports for the commit 67741f8 to get more accurate results

@@            Coverage Diff             @@
##           master    #3102      +/-   ##
==========================================
+ Coverage   85.93%   85.95%   +0.01%     
==========================================
  Files         289      290       +1     
  Lines       52101    52270     +169     
==========================================
+ Hits        44774    44927     +153     
- Misses       7327     7343      +16     
Impacted Files Coverage Δ
...usion/core/src/avro_to_arrow/arrow_array_reader.rs 0.00% <0.00%> (ø)
datafusion/core/src/catalog/information_schema.rs 94.42% <0.00%> (ø)
datafusion/core/src/datasource/listing/mod.rs 55.55% <ø> (ø)
...afusion/core/src/physical_plan/file_format/avro.rs 0.00% <ø> (ø)
datafusion/core/src/physical_plan/hash_utils.rs 40.43% <0.00%> (ø)
datafusion/core/src/physical_plan/repartition.rs 93.26% <ø> (ø)
...tafusion/core/src/physical_plan/sort_merge_join.rs 90.36% <0.00%> (ø)
datafusion/core/tests/sql/aggregates.rs 99.36% <ø> (ø)
datafusion/core/tests/sql/decimal.rs 100.00% <ø> (ø)
datafusion/core/tests/sql/joins.rs 99.33% <ø> (ø)
... and 51 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

codecov-commenter avatar Aug 11 '22 04:08 codecov-commenter

LGTM @turbo1912. Could you update the rustdoc at the top of the file now that this optimizes both limits and offsets? I think it makes sense for these related optimizations to be in the same rule.

andygrove avatar Aug 11 '22 13:08 andygrove

Nice -- thanks @turbo1912 !

alamb avatar Aug 11 '22 21:08 alamb

Benchmark runs are scheduled for baseline = 9956f80f197550051db7debae15d5c706afc22a3 and contender = dbfb0e6ff9bc6fb933ed7cfd15bd9127ac0d443a. dbfb0e6ff9bc6fb933ed7cfd15bd9127ac0d443a is a master commit associated with this PR. Results will be available as each benchmark for each run completes. Conbench compare runs links: [Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2 [Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm [Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x [Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q Buildkite builds: Supported benchmarks: ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True test-mac-arm: Supported benchmark langs: C++, Python, R ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

ursabot avatar Aug 11 '22 21:08 ursabot