Account for constant equivalence properties in union, tests #12562

alamb · 2024-09-20T22:18:24Z

Which issue does this PR close?

Rationale for this change

Fixes a bug: #12446

Note it doesn't include end to end tests as they hit a stack overflow (tracked by #12700)

I don't think we should merge this PR until #12700 is fixed as that would turn what is now a plan runtime error into a stack overflow for certain expressions, which seems like a regression to me

What changes are included in this PR?

Update the code that calculates the output EquivalenceProperties for a union to account for constants and "augment" the sort orders
Add additional tests

Are these changes tested?

Yes

Are there any user-facing changes?

Fixed bug

alamb · 2024-09-20T22:20:07Z

datafusion/physical-expr/src/equivalence/properties.rs

-        let test_cases = vec![
-            // --------- TEST CASE 1 ----------
-            (
+        UnionEquivalenceTest::new(&schema)


I wanted more flexibility with the tests, and I am glad I did this as I hadn't seen the existing tests for constants

alamb · 2024-10-02T15:34:33Z

datafusion/physical-expr/src/equivalence/properties.rs

-    }
-    let mut eq_properties = EquivalenceProperties::new(lhs.schema);
-    eq_properties.constants = constants;
+    let mut orderings = UnionEquivalentOrderingBuilder::new();


this is the key change that encapsulates the sort expression calculation

alamb · 2024-10-02T15:35:32Z

datafusion/physical-expr/src/equivalence/properties.rs

                vec![vec!["a"]],
                vec![],
            )
            .run()
    }

    #[test]
-    #[ignore]
-    // ignored due to https://github.com/apache/datafusion/issues/12446


the tests now pass 🎉 -- I also renamed them to try and be more specific about what they are testing

alamb · 2024-10-02T15:42:16Z

datafusion/physical-expr/src/equivalence/class.rs

+    }
+
+    /// Returns a [`Display`]able list of `ConstExpr`.
+    pub fn format_list(input: &[ConstExpr]) -> impl Display + '_ {


this is just moved into a function from EquivalenceProperties

alamb · 2024-10-02T15:57:04Z

datafusion/physical-expr/src/equivalence/properties.rs

-                ),
-            );
+
+            // try all permutations of the children


I also updated the test runner to test with all child permutations (aka different orders) as the algorithm is not obviously symmetric

alamb · 2024-10-02T15:58:06Z

datafusion/physical-expr/src/equivalence/properties.rs

@@ -1675,8 +1679,7 @@ pub fn calculate_union(
    eqps: Vec<EquivalenceProperties>,
    schema: SchemaRef,
 ) -> Result<EquivalenceProperties> {
-    // TODO: In some cases, we should be able to preserve some equivalence
-    //       classes. Add support for such cases.
+    // TODO avoid this clone by using `reduce`


berkaysynnada

Thanks @alamb for this great algorithm. I have tested a few tricky cases, and all gave the expected results. The new tests have also improved the coverage. Maybe @akurmustafa could check if we missed anything, but I believe it is solid and aligns with what I had in mind.

alamb · 2024-10-04T12:47:40Z

Sounds good -- thank you for the review @berkaysynnada

I am currently working on #12700 which I think should be fixed too prior to merging this PR.

alamb · 2024-10-07T10:30:48Z

Now that #12700 has been fixed, I have merged this PR up from main and plan to merge it later today unless anyone would like additional time to comment

I am also happy to make changes as follow on PRs if needed

alamb · 2024-10-07T11:19:02Z

I think CI / clippy will be fixed by #12724

alamb · 2024-10-07T17:29:22Z

Thanks again @berkaysynnada

@etseidl

* Add support for external tables with qualified names (#12645) * Make support schemas * Set default name to table * Remove print statements and stale comment * Add tests for create table * Fix typo * Update datafusion/sql/src/statement.rs Co-authored-by: Jonah Gao <[email protected]> * convert create_external_table to objectname * Add sqllogic tests * Fix failing tests --------- Co-authored-by: Jonah Gao <[email protected]> * Fix Regex signature types (#12690) * Fix Regex signature types * Uncomment the shared tests in string_query.slt.part and removed tests copies everywhere else * Test `LIKE` and `MATCH` with flags; Remove new tests from regexp.slt * Refactor `ByteGroupValueBuilder` to use `MaybeNullBufferBuilder` (#12681) * Fix malformed hex string literal in docs (#12708) * Simplify match patterns in coercion rules (#12711) Remove conditions where unnecessary. Refactor to improve readability. * Remove aggregate functions dependency on frontend (#12715) * Remove aggregate functions dependency on frontend DataFusion is a SQL query engine and also a reusable library for building query engines. The core functionality should not depend on frontend related functionalities like `sqlparser` or `datafusion-sql`. * Remove duplicate license header * Minor: Remove clone in `transform_to_states` (#12707) * rm clone Signed-off-by: jayzhan211 <[email protected]> * fmt Signed-off-by: jayzhan211 <[email protected]> --------- Signed-off-by: jayzhan211 <[email protected]> * Refactor tests for union sorting properties, add tests for unions and constants (#12702) * Refactor tests for union sorting properties * update doc test * Undo import reordering * remove unecessary static lifetimes * Fix: support Qualified Wildcard in count aggregate function (#12673) * Reduce code duplication in `PrimitiveGroupValueBuilder` with const generics (#12703) * Reduce code duplication in `PrimitiveGroupValueBuilder` with const generics * Fix docs * Disallow duplicated qualified field names (#12608) * Disallow duplicated qualified field names * Fix tests * Optimize base64/hex decoding by pre-allocating output buffers (~2x faster) (#12675) * add bench * replace macro with generic function * remove duplicated code * optimize base64/hex decode * Allow DynamicFileCatalog support to query partitioned file (#12683) * support to query partitioned table for dynamic file catalog * cargo clippy * split partitions inferring to another function * Support `LIMIT` Push-down logical plan optimization for `Extension` nodes (#12685) * Update trait `UserDefinedLogicalNodeCore` Signed-off-by: Austin Liu <[email protected]> * Update corresponding interface Signed-off-by: Austin Liu <[email protected]> Add rewrite rule for `push-down-limit` for `Extension` Signed-off-by: Austin Liu <[email protected]> * Add rewrite rule for `push-down-limit` for `Extension` and tests Signed-off-by: Austin Liu <[email protected]> * Update corresponding interface Signed-off-by: Austin Liu <[email protected]> * Reorganize to match guard Signed-off-by: Austin Liu <[email protected]> * Clena up Signed-off-by: Austin Liu <[email protected]> Clean up Signed-off-by: Austin Liu <[email protected]> --------- Signed-off-by: Austin Liu <[email protected]> * Fix AvroReader: Add union resolving for nested struct arrays (#12686) * Add union resolving for nested struct arrays * Add test * Change test * Reproduce index error * fmt --------- Co-authored-by: Andrew Lamb <[email protected]> * Adds macros for creating `WindowUDF` and `WindowFunction` expression (#12693) * Adds macro for udwf singleton * Adds a doc comment parameter to macro * Add doc comment for `create_udwf` macro * Uses default constructor * Update `Cargo.lock` in `datafusion-cli` * Fixes: expand `$FN_NAME` in doc strings * Adds example for macro usage * Renames macro * Improve doc comments * Rename udwf macro * Minor: doc copy edits * Adds macro for creating fluent-style expression API * Adds support for 1 or more parameters in expression function * Rewrite doc comments * Rename parameters * Minor: formatting * Adds doc comment for `create_udwf_expr` macro * Improve example docs * Hides extraneous code in doc comments * Add a one-line readme * Adds doc test assertions + minor formatting fixes * Adds common macro for defining user-defined window functions * Adds doc comment for `define_udwf_and_expr` * Defines `RowNumber` using common macro * Add usage example for common macro * Adds usage for custom constructor * Add examples for remaining patterns * Improve doc comments for usage examples * Rewrite inner line docs * Rewrite `create_udwf_expr!` doc comments * Minor doc improvements * Fix doc test and usage example * Add inline comments for macro patterns * Minor: change doc comment in example * Support unparsing plans with both Aggregation and Window functions (#12705) * Support unparsing plans with both Aggregation and Window functions (#35) * Fix unparsing for aggregation grouping sets * Add test for grouping set unparsing * Update datafusion/sql/src/unparser/utils.rs Co-authored-by: Jax Liu <[email protected]> * Update datafusion/sql/src/unparser/utils.rs Co-authored-by: Jax Liu <[email protected]> * Update * More tests --------- Co-authored-by: Jax Liu <[email protected]> * Fix strpos invocation with dictionary and null (#12712) In 1b3608d `strpos` signature was modified to indicate it supports dictionary as input argument, but the invoke method doesn't support them. * docs: Update DataFusion introduction to clarify that DataFusion does provide an "out of the box" query engine (#12666) * Update DataFusion introduction to show that DataFusion offers packaged versions for end users * change order * Update README.md Co-authored-by: Andrew Lamb <[email protected]> * refine wording and update user guide for consistency * prettier --------- Co-authored-by: Andrew Lamb <[email protected]> * Framework for generating function docs from embedded code documentation (#12668) * Initial work on #12432 to allow for generation of udf docs from embedded documentation in the code * Add missing license header. * Fixed examples. * Fixing a really weird RustRover/wsl ... something. No clue what happened there. * permission change * Cargo fmt update. * Refactored Documentation to allow it to be used in a const. * Add documentation for syntax_example * Refactoring Documentation based on PR feedback. * Cargo fmt update. * Doc update * Fixed copy/paste error. * Minor text updates. --------- Co-authored-by: Andrew Lamb <[email protected]> * Add IMDB(JOB) Benchmark [2/N] (imdb queries) (#12529) * imdb dataset * cargo fmt * Add 113 queries for IMDB(JOB) Signed-off-by: Austin Liu <[email protected]> * Add `get_query_sql` from `query_id` string Signed-off-by: Austin Liu <[email protected]> * Fix CSV reader & Remove Parquet partition Signed-off-by: Austin Liu <[email protected]> * Add benchmark IMDB runner Signed-off-by: Austin Liu <[email protected]> * Add `run_imdb` script Signed-off-by: Austin Liu <[email protected]> * Add checker for imdb option Signed-off-by: Austin Liu <[email protected]> * Add SLT for IMDB Signed-off-by: Austin Liu <[email protected]> * Fix `get_query_sql()` for CI roundtrip test Signed-off-by: Austin Liu <[email protected]> Fix `get_query_sql()` for CI roundtrip test Signed-off-by: Austin Liu <[email protected]> Fix `get_query_sql()` for CI roundtrip test Signed-off-by: Austin Liu <[email protected]> * Clean up Signed-off-by: Austin Liu <[email protected]> * Add missing license Signed-off-by: Austin Liu <[email protected]> * Add IMDB(JOB) queries `2b` to `5c` Signed-off-by: Austin Liu <[email protected]> * Add `INCLUDE_IMDB` in CI verify-benchmark-results Signed-off-by: Austin Liu <[email protected]> * Prepare IMDB dataset Signed-off-by: Austin Liu <[email protected]> Prepare IMDB dataset Signed-off-by: Austin Liu <[email protected]> * use uint as id type * format * Seperate `tpch` and `imdb` benchmarking CI jobs Signed-off-by: Austin Liu <[email protected]> Fix path Signed-off-by: Austin Liu <[email protected]> Fix path Signed-off-by: Austin Liu <[email protected]> Remove `tpch` in `imdb` benchmark Signed-off-by: Austin Liu <[email protected]> * Remove IMDB(JOB) slt in CI Signed-off-by: Austin Liu <[email protected]> Remove IMDB(JOB) slt in CI Signed-off-by: Austin Liu <[email protected]> --------- Signed-off-by: Austin Liu <[email protected]> Co-authored-by: DouPache <[email protected]> * Minor: avoid clone while calculating union equivalence properties (#12722) * Minor: avoid clone while calculating union equivalence properties * Update datafusion/physical-expr/src/equivalence/properties.rs * fmt * Simplify streaming_merge function parameters (#12719) * simplify streaming_merge function parameters * revert test change * change StreamingMergeConfig into builder pattern * Fix links on docs index page (#12750) * Provide field and schema metadata missing on cross joins, and union with null fields. (#12729) * test: reproducer for missing schema metadata on cross join * fix: pass thru schema metadata on cross join * fix: preserve metadata when transforming to view types * test: reproducer for missing field metadata in left hand NULL field of union * fix: preserve field metadata from right side of union * chore: safe indexing * Minor: Update string tests for strpos (#12739) * Apply `type_union_resolution` to array and values (#12753) * cleanup make array coercion rule Signed-off-by: jayzhan211 <[email protected]> * change to type union resolution Signed-off-by: jayzhan211 <[email protected]> * change value too Signed-off-by: jayzhan211 <[email protected]> * fix tpyo Signed-off-by: jayzhan211 <[email protected]> --------- Signed-off-by: jayzhan211 <[email protected]> * Add `DocumentationBuilder::with_standard_argument` to reduce copy/paste (#12747) * Add `DocumentationBuilder::with_standard_expression` to reduce copy/paste * fix doc * fix standard argument * Update docs * Improve documentation to explain what is different * fix `equal_to` in `PrimitiveGroupValueBuilder` (#12758) * fix `equal_to` in `PrimitiveGroupValueBuilder`. * fix typo. * add uts. * reduce calling of `is_null`. * Minor: doc how field name is to be set (#12757) * Fix `equal_to` in `ByteGroupValueBuilder` (#12770) * Fix `equal_to` in `ByteGroupValueBuilder` * refactor null_equal_to * Update datafusion/physical-plan/src/aggregates/group_values/group_column.rs * Allow simplification even when nullable (#12746) The nullable requirement seem to have been added in #1401 but as far as I can tell they are not needed for these 2 cases. I think this can be shown using this truth table: (generated using datafusion-cli without this patch) ``` > CREATE TABLE t (v BOOLEAN) as values (true), (false), (NULL); > select t.v, t2.v, t.v AND (t.v OR t2.v), t.v OR (t.v AND t2.v) from t cross join t as t2; +-------+-------+---------------------+---------------------+ | v | v | t.v AND t.v OR t2.v | t.v OR t.v AND t2.v | +-------+-------+---------------------+---------------------+ | true | true | true | true | | true | false | true | true | | true | | true | true | | false | true | false | false | | false | false | false | false | | false | | false | false | | | true | | | | | false | | | | | | | | +-------+-------+---------------------+---------------------+ ``` And it seems Spark applies both of these and DuckDB applies only the first one. * Fix unnest conjunction with selecting wildcard expression (#12760) * fix unnest statement with wildcard expression * add commnets * Improve `round` scalar function unparsing for Postgres (#12744) * Postgres: enforce required `NUMERIC` type for `round` scalar function (#34) Includes initial support for dialects to override scalar functions unparsing * Document scalar_function_to_sql_overrides fn * Fix stack overflow calculating projected orderings (#12759) * Fix stack overflow calculating projected orderings * fix docs * Port / Add Documentation for `VarianceSample` and `VariancePopulation` (#12742) * Upgrade arrow/parquet to `53.1.0` / fix clippy (#12724) * Update to arrow/parquet 53.1.0 * Update some API * update for changed file sizes * Use non deprecated APIs * Use ParquetMetadataReader from @etseidl * remove upstreamed implementation * Update CSV schema * Use upstream is_null and is_not_null kernels * feat: add support for Substrait ExtendedExpression (#12728) * Add support for serializing and deserializing Substrait ExtendedExpr message * Address clippy reviews * Reuse existing rename method * Transformed::new_transformed: Fix documentation formatting (#12787) Co-authored-by: Andrew Lamb <[email protected]> * fix: Correct results for grouping sets when columns contain nulls (#12571) * Fix grouping sets behavior when data contains nulls * PR suggestion comment * Update new test case * Add grouping_id to the logical plan * Add doc comment next to INTERNAL_GROUPING_ID * Fix unparsing of Aggregate with grouping sets --------- Co-authored-by: Andrew Lamb <[email protected]> * Migrate documentation for all string functions from scalar_functions.md to code (#12775) * Added documentation for string and unicode functions. * Fixed issues with aliases. * Cargo fmt. * Minor doc fixes. * Update docs for var_pop/samp --------- Co-authored-by: Andrew Lamb <[email protected]> * Account for constant equivalence properties in union, tests (#12562) * Minor: clarify comment about empty dependencies (#12786) * Introduce Signature::String and return error if input of `strpos` is integer (#12751) * fix sig Signed-off-by: jayzhan211 <[email protected]> * fix Signed-off-by: jayzhan211 <[email protected]> * fix error Signed-off-by: jayzhan211 <[email protected]> * fix all signature Signed-off-by: jayzhan211 <[email protected]> * fix all signature Signed-off-by: jayzhan211 <[email protected]> * change default type Signed-off-by: jayzhan211 <[email protected]> * clippy Signed-off-by: jayzhan211 <[email protected]> * fix docs Signed-off-by: jayzhan211 <[email protected]> * rm deadcode Signed-off-by: jayzhan211 <[email protected]> * cleanup Signed-off-by: jayzhan211 <[email protected]> * cleanup Signed-off-by: jayzhan211 <[email protected]> * rm test Signed-off-by: jayzhan211 <[email protected]> --------- Signed-off-by: jayzhan211 <[email protected]> * Minor: improve docs on MovingMin/MovingMax (#12790) * Add slt tests (#12721) --------- Signed-off-by: jayzhan211 <[email protected]> Signed-off-by: Austin Liu <[email protected]> Co-authored-by: OussamaSaoudi <[email protected]> Co-authored-by: Jonah Gao <[email protected]> Co-authored-by: Dmitrii Blaginin <[email protected]> Co-authored-by: Andrew Lamb <[email protected]> Co-authored-by: Tomoaki Kawada <[email protected]> Co-authored-by: Piotr Findeisen <[email protected]> Co-authored-by: Jay Zhan <[email protected]> Co-authored-by: HuSen <[email protected]> Co-authored-by: Emil Ejbyfeldt <[email protected]> Co-authored-by: Simon Vandel Sillesen <[email protected]> Co-authored-by: Jax Liu <[email protected]> Co-authored-by: Austin Liu <[email protected]> Co-authored-by: JonasDev1 <[email protected]> Co-authored-by: jcsherin <[email protected]> Co-authored-by: Sergei Grebnov <[email protected]> Co-authored-by: Andy Grove <[email protected]> Co-authored-by: Bruce Ritchie <[email protected]> Co-authored-by: DouPache <[email protected]> Co-authored-by: mertak-synnada <[email protected]> Co-authored-by: Bryce Mecum <[email protected]> Co-authored-by: wiedld <[email protected]> Co-authored-by: kamille <[email protected]> Co-authored-by: Weston Pace <[email protected]> Co-authored-by: Val Lorentz <[email protected]>

github-actions bot added physical-expr Physical Expressions sqllogictest SQL Logic Tests (.slt) labels Sep 20, 2024

alamb mentioned this pull request Sep 20, 2024

SanityChecker rejects certain valid UNION plans #12446

Closed

alamb commented Sep 20, 2024

View reviewed changes

alamb mentioned this pull request Sep 23, 2024

Add Docs and Examples and helper methods to PhysicalSortExpr #12589

Merged

alamb force-pushed the alamb/union_sorting branch from d633a19 to 8edc8de Compare September 23, 2024 12:59

github-actions bot added the core Core DataFusion crate label Sep 23, 2024

alamb mentioned this pull request Sep 23, 2024

Improve documentation and add Display impl to EquivalenceProperties #12590

Merged

alamb force-pushed the alamb/union_sorting branch from 8edc8de to 2fe21c8 Compare September 23, 2024 13:25

alamb mentioned this pull request Sep 23, 2024

Add additional documentation and builder APIs to SortOptions apache/arrow-rs#6441

Merged

alamb force-pushed the alamb/union_sorting branch 2 times, most recently from b72a1d1 to 48bebcb Compare September 25, 2024 23:05

github-actions bot removed the core Core DataFusion crate label Sep 25, 2024

alamb force-pushed the alamb/union_sorting branch 4 times, most recently from 3ed9a9f to 0d545c9 Compare October 1, 2024 11:17

This was referenced Oct 1, 2024

Stack overflow calling EqvalenceProperties::project with certain order #12700

Closed

Refactor tests for union sorting properties, add tests for unions and constants #12702

Merged

alamb force-pushed the alamb/union_sorting branch 3 times, most recently from cee3f3b to 09a9fa0 Compare October 2, 2024 15:24

alamb mentioned this pull request Oct 2, 2024

Add union sorting equivalence end to end tests #12721

Merged

alamb changed the title ~~WIP: Account for constant equivalence properties in union, tests~~ Account for constant equivalence properties in union, tests Oct 2, 2024

alamb force-pushed the alamb/union_sorting branch from 09a9fa0 to dcb9b34 Compare October 2, 2024 15:32

github-actions bot removed the sqllogictest SQL Logic Tests (.slt) label Oct 2, 2024

alamb commented Oct 2, 2024

View reviewed changes

alamb force-pushed the alamb/union_sorting branch from dcb9b34 to bf5828f Compare October 2, 2024 15:36

Account for constant equivalence properties in union, tests

c1ee59b

alamb force-pushed the alamb/union_sorting branch from bf5828f to c1ee59b Compare October 2, 2024 15:41

alamb commented Oct 2, 2024

View reviewed changes

alamb mentioned this pull request Oct 2, 2024

Minor: avoid clone while calculating union equivalence properties #12722

Merged

alamb commented Oct 2, 2024

View reviewed changes

alamb marked this pull request as ready for review October 2, 2024 16:03

berkaysynnada approved these changes Oct 4, 2024

View reviewed changes

alamb added 2 commits October 4, 2024 16:12

Merge remote-tracking branch 'apache/main' into alamb/union_sorting

6add851

Merge remote-tracking branch 'apache/main' into alamb/union_sorting

36211c8

Merge remote-tracking branch 'apache/main' into alamb/union_sorting

6ad5b0b

alamb merged commit 577e4bb into apache:main Oct 7, 2024
24 checks passed

alamb deleted the alamb/union_sorting branch October 7, 2024 17:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Account for constant equivalence properties in union, tests #12562

Account for constant equivalence properties in union, tests #12562

alamb commented Sep 20, 2024 •

edited

Loading

alamb Sep 20, 2024

alamb Oct 2, 2024

alamb Oct 2, 2024

alamb Oct 2, 2024

alamb Oct 2, 2024

alamb Oct 2, 2024

berkaysynnada left a comment

alamb commented Oct 4, 2024

alamb commented Oct 7, 2024

alamb commented Oct 7, 2024

alamb commented Oct 7, 2024

Account for constant equivalence properties in union, tests #12562

Account for constant equivalence properties in union, tests #12562

Conversation

alamb commented Sep 20, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

alamb Sep 20, 2024

Choose a reason for hiding this comment

alamb Oct 2, 2024

Choose a reason for hiding this comment

alamb Oct 2, 2024

Choose a reason for hiding this comment

alamb Oct 2, 2024

Choose a reason for hiding this comment

alamb Oct 2, 2024

Choose a reason for hiding this comment

alamb Oct 2, 2024

Choose a reason for hiding this comment

berkaysynnada left a comment

Choose a reason for hiding this comment

alamb commented Oct 4, 2024

alamb commented Oct 7, 2024

alamb commented Oct 7, 2024

alamb commented Oct 7, 2024

alamb commented Sep 20, 2024 •

edited

Loading