From 7afeb8b8078018a167e7bce6421024b95517b234 Mon Sep 17 00:00:00 2001 From: Asura7969 <1402357969@qq.com> Date: Wed, 8 Nov 2023 17:04:53 +0800 Subject: [PATCH 1/9] Minor: Improve the document format of JoinHashMap --- .../src/joins/hash_join_utils.rs | 115 ++++++++++-------- 1 file changed, 62 insertions(+), 53 deletions(-) diff --git a/datafusion/physical-plan/src/joins/hash_join_utils.rs b/datafusion/physical-plan/src/joins/hash_join_utils.rs index 3a2a85c727226..3ea0331ab4fe8 100644 --- a/datafusion/physical-plan/src/joins/hash_join_utils.rs +++ b/datafusion/physical-plan/src/joins/hash_join_utils.rs @@ -40,59 +40,68 @@ use datafusion_physical_expr::{PhysicalExpr, PhysicalSortExpr}; use hashbrown::raw::RawTable; use hashbrown::HashSet; -// Maps a `u64` hash value based on the build side ["on" values] to a list of indices with this key's value. -// By allocating a `HashMap` with capacity for *at least* the number of rows for entries at the build side, -// we make sure that we don't have to re-hash the hashmap, which needs access to the key (the hash in this case) value. -// E.g. 1 -> [3, 6, 8] indicates that the column values map to rows 3, 6 and 8 for hash value 1 -// As the key is a hash value, we need to check possible hash collisions in the probe stage -// During this stage it might be the case that a row is contained the same hashmap value, -// but the values don't match. Those are checked in the [equal_rows] macro -// The indices (values) are stored in a separate chained list stored in the `Vec`. -// The first value (+1) is stored in the hashmap, whereas the next value is stored in array at the position value. -// The chain can be followed until the value "0" has been reached, meaning the end of the list. -// Also see chapter 5.3 of [Balancing vectorized query execution with bandwidth-optimized storage](https://dare.uva.nl/search?identifier=5ccbb60a-38b8-4eeb-858a-e7735dd37487) -// See the example below: -// Insert (1,1) -// map: -// --------- -// | 1 | 2 | -// --------- -// next: -// --------------------- -// | 0 | 0 | 0 | 0 | 0 | -// --------------------- -// Insert (2,2) -// map: -// --------- -// | 1 | 2 | -// | 2 | 3 | -// --------- -// next: -// --------------------- -// | 0 | 0 | 0 | 0 | 0 | -// --------------------- -// Insert (1,3) -// map: -// --------- -// | 1 | 4 | -// | 2 | 3 | -// --------- -// next: -// --------------------- -// | 0 | 0 | 0 | 2 | 0 | <--- hash value 1 maps to 4,2 (which means indices values 3,1) -// --------------------- -// Insert (1,4) -// map: -// --------- -// | 1 | 5 | -// | 2 | 3 | -// --------- -// next: -// --------------------- -// | 0 | 0 | 0 | 2 | 4 | <--- hash value 1 maps to 5,4,2 (which means indices values 4,3,1) -// --------------------- -// TODO: speed up collision checks -// https://github.com/apache/arrow-datafusion/issues/50 +/// Maps a `u64` hash value based on the build side ["on" values] to a list of indices with this key's value. +/// +/// By allocating a `HashMap` with capacity for *at least* the number of rows for entries at the build side, +/// we make sure that we don't have to re-hash the hashmap, which needs access to the key (the hash in this case) value. +/// +/// E.g. 1 -> [3, 6, 8] indicates that the column values map to rows 3, 6 and 8 for hash value 1 +/// As the key is a hash value, we need to check possible hash collisions in the probe stage +/// During this stage it might be the case that a row is contained the same hashmap value, +/// but the values don't match. Those are checked in the [equal_rows] macro +/// The indices (values) are stored in a separate chained list stored in the `Vec`. +/// +/// The first value (+1) is stored in the hashmap, whereas the next value is stored in array at the position value. +/// +/// The chain can be followed until the value "0" has been reached, meaning the end of the list. +/// Also see chapter 5.3 of [Balancing vectorized query execution with bandwidth-optimized storage](https://dare.uva.nl/search?identifier=5ccbb60a-38b8-4eeb-858a-e7735dd37487) +/// +/// # Example +/// +/// ``` text +/// See the example below: +/// Insert (1,1) +/// map: +/// --------- +/// | 1 | 2 | +/// --------- +/// next: +/// --------------------- +/// | 0 | 0 | 0 | 0 | 0 | +/// --------------------- +/// Insert (2,2) +/// map: +/// --------- +/// | 1 | 2 | +/// | 2 | 3 | +/// --------- +/// next: +/// --------------------- +/// | 0 | 0 | 0 | 0 | 0 | +/// --------------------- +/// Insert (1,3) +/// map: +/// --------- +/// | 1 | 4 | +/// | 2 | 3 | +/// --------- +/// next: +/// --------------------- +/// | 0 | 0 | 0 | 2 | 0 | <--- hash value 1 maps to 4,2 (which means indices values 3,1) +/// --------------------- +/// Insert (1,4) +/// map: +/// --------- +/// | 1 | 5 | +/// | 2 | 3 | +/// --------- +/// next: +/// --------------------- +/// | 0 | 0 | 0 | 2 | 4 | <--- hash value 1 maps to 5,4,2 (which means indices values 4,3,1) +/// --------------------- +/// ``` +/// +///TODO: [speed up collision checks](https://github.com/apache/arrow-datafusion/issues/50) pub struct JoinHashMap { // Stores hash value to last row index pub map: RawTable<(u64, u64)>, From 9e6548205352789acc1b96253108e32d3bc74ca1 Mon Sep 17 00:00:00 2001 From: asura7969 <1402357969@qq.com> Date: Tue, 5 Dec 2023 00:23:11 +0800 Subject: [PATCH 2/9] ambiguous reference --- datafusion/expr/src/expr.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/datafusion/expr/src/expr.rs b/datafusion/expr/src/expr.rs index ee9b0ad6f967a..64fb0a44e5eb1 100644 --- a/datafusion/expr/src/expr.rs +++ b/datafusion/expr/src/expr.rs @@ -911,6 +911,10 @@ impl Expr { asc, nulls_first, }) => Expr::Sort(Sort::new(Box::new(expr.alias(name)), asc, nulls_first)), + Expr::Column(Column { ref relation, .. }) => { + let expr = self.clone(); + Expr::Alias(Alias::new(expr, relation.clone(), name.into())) + } _ => Expr::Alias(Alias::new(self, None::<&str>, name.into())), } } From 443595cf8f3501514b5ff53ac8215871f1bbae4e Mon Sep 17 00:00:00 2001 From: Asura7969 <1402357969@qq.com> Date: Tue, 5 Dec 2023 16:15:00 +0800 Subject: [PATCH 3/9] ignore aliases --- datafusion/sql/src/select.rs | 6 +++++- datafusion/sql/tests/sql_integration.rs | 6 +++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/datafusion/sql/src/select.rs b/datafusion/sql/src/select.rs index c546ca755206e..de660035ce000 100644 --- a/datafusion/sql/src/select.rs +++ b/datafusion/sql/src/select.rs @@ -384,7 +384,11 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> { &[&[plan.schema()]], &plan.using_columns()?, )?; - let expr = col.alias(self.normalizer.normalize(alias)); + let name = self.normalizer.normalize(alias); + let expr = match &col { + Expr::Column(column) if column.name.eq(&name) => col, + _ => col.alias(name), + }; Ok(vec![expr]) } SelectItem::Wildcard(options) => { diff --git a/datafusion/sql/tests/sql_integration.rs b/datafusion/sql/tests/sql_integration.rs index d5b06bcf815fc..d04cfec3157d5 100644 --- a/datafusion/sql/tests/sql_integration.rs +++ b/datafusion/sql/tests/sql_integration.rs @@ -3542,9 +3542,9 @@ fn test_select_unsupported_syntax_errors(#[case] sql: &str, #[case] error: &str) fn select_order_by_with_cast() { let sql = "SELECT first_name AS first_name FROM (SELECT first_name AS first_name FROM person) ORDER BY CAST(first_name as INT)"; - let expected = "Sort: CAST(first_name AS first_name AS Int32) ASC NULLS LAST\ - \n Projection: first_name AS first_name\ - \n Projection: person.first_name AS first_name\ + let expected = "Sort: CAST(person.first_name AS Int32) ASC NULLS LAST\ + \n Projection: person.first_name\ + \n Projection: person.first_name\ \n TableScan: person"; quick_test(sql, expected); } From dd5af6dc199cc7b75b2f7381353ead1c756dae56 Mon Sep 17 00:00:00 2001 From: asura7969 <1402357969@qq.com> Date: Tue, 5 Dec 2023 20:20:29 +0800 Subject: [PATCH 4/9] fix --- datafusion/optimizer/tests/optimizer_integration.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/datafusion/optimizer/tests/optimizer_integration.rs b/datafusion/optimizer/tests/optimizer_integration.rs index e593b07361e2c..4172881c0aad0 100644 --- a/datafusion/optimizer/tests/optimizer_integration.rs +++ b/datafusion/optimizer/tests/optimizer_integration.rs @@ -324,11 +324,10 @@ fn push_down_filter_groupby_expr_contains_alias() { fn test_same_name_but_not_ambiguous() { let sql = "SELECT t1.col_int32 AS col_int32 FROM test t1 intersect SELECT col_int32 FROM test t2"; let plan = test_sql(sql).unwrap(); - let expected = "LeftSemi Join: col_int32 = t2.col_int32\ - \n Aggregate: groupBy=[[col_int32]], aggr=[[]]\ - \n Projection: t1.col_int32 AS col_int32\ - \n SubqueryAlias: t1\ - \n TableScan: test projection=[col_int32]\ + let expected = "LeftSemi Join: t1.col_int32 = t2.col_int32\ + \n Aggregate: groupBy=[[t1.col_int32]], aggr=[[]]\ + \n SubqueryAlias: t1\ + \n TableScan: test projection=[col_int32]\ \n SubqueryAlias: t2\ \n TableScan: test projection=[col_int32]"; assert_eq!(expected, format!("{plan:?}")); From d755ce6b19e07c5ab915f7c7c9b6ebddfc002795 Mon Sep 17 00:00:00 2001 From: asura7969 <1402357969@qq.com> Date: Tue, 5 Dec 2023 20:25:31 +0800 Subject: [PATCH 5/9] fix --- datafusion/expr/src/expr.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/datafusion/expr/src/expr.rs b/datafusion/expr/src/expr.rs index 1d9e9703f0a67..6fa400454dff4 100644 --- a/datafusion/expr/src/expr.rs +++ b/datafusion/expr/src/expr.rs @@ -911,10 +911,6 @@ impl Expr { asc, nulls_first, }) => Expr::Sort(Sort::new(Box::new(expr.alias(name)), asc, nulls_first)), - Expr::Column(Column { ref relation, .. }) => { - let expr = self.clone(); - Expr::Alias(Alias::new(expr, relation.clone(), name.into())) - } _ => Expr::Alias(Alias::new(self, None::<&str>, name.into())), } } From a338ac22767fa277ddf514e144d7ebb21b204218 Mon Sep 17 00:00:00 2001 From: Asura7969 <1402357969@qq.com> Date: Wed, 6 Dec 2023 10:42:31 +0800 Subject: [PATCH 6/9] add test --- datafusion/sql/src/select.rs | 1 + datafusion/sql/tests/sql_integration.rs | 11 +++++++++++ datafusion/sqllogictest/test_files/select.slt | 18 ++++++++++++++++++ 3 files changed, 30 insertions(+) diff --git a/datafusion/sql/src/select.rs b/datafusion/sql/src/select.rs index de660035ce000..15f720d75652a 100644 --- a/datafusion/sql/src/select.rs +++ b/datafusion/sql/src/select.rs @@ -385,6 +385,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> { &plan.using_columns()?, )?; let name = self.normalizer.normalize(alias); + // avoiding adding an alias if the column name is the same. let expr = match &col { Expr::Column(column) if column.name.eq(&name) => col, _ => col.alias(name), diff --git a/datafusion/sql/tests/sql_integration.rs b/datafusion/sql/tests/sql_integration.rs index 88f2bf2a1925a..c06d0fe8b08ac 100644 --- a/datafusion/sql/tests/sql_integration.rs +++ b/datafusion/sql/tests/sql_integration.rs @@ -3553,6 +3553,17 @@ fn select_order_by_with_cast() { quick_test(sql, expected); } +#[test] +fn test_avoid_add_alias() { + // avoiding adding an alias if the column name is the same. + // plan1 = plan2 + let sql = "select person.id as id from person order by person.id"; + let plan1 = logical_plan(sql).unwrap(); + let sql = "select id from person order by id"; + let plan2 = logical_plan(sql).unwrap(); + assert_eq!(format!("{plan1:?}"), format!("{plan2:?}")); +} + #[test] fn test_duplicated_left_join_key_inner_join() { // person.id * 2 happen twice in left side. diff --git a/datafusion/sqllogictest/test_files/select.slt b/datafusion/sqllogictest/test_files/select.slt index bb81c5a9a1383..646a1db99e8ae 100644 --- a/datafusion/sqllogictest/test_files/select.slt +++ b/datafusion/sqllogictest/test_files/select.slt @@ -868,6 +868,24 @@ statement error DataFusion error: Error during planning: EXCLUDE or EXCEPT conta SELECT * EXCLUDE(d, b, c, a, a, b, c, d) FROM table1 +# avoiding adding an alias if the column name is the same +query I +select a as a FROM table1 order by a +---- +1 +2 + +query TT +EXPLAIN select a as a FROM table1 order by a +---- +logical_plan +Sort: table1.a ASC NULLS LAST +--TableScan: table1 projection=[a] +physical_plan +SortExec: expr=[a@0 ASC NULLS LAST] +--MemoryExec: partitions=1, partition_sizes=[1] + + # run below query in multi partitions statement ok set datafusion.execution.target_partitions = 2; From 47b81e6e5a1e29f756c41b052e213dcb8ff02a6a Mon Sep 17 00:00:00 2001 From: Asura7969 <1402357969@qq.com> Date: Wed, 6 Dec 2023 10:43:38 +0800 Subject: [PATCH 7/9] add test --- datafusion/sqllogictest/test_files/select.slt | 1 - 1 file changed, 1 deletion(-) diff --git a/datafusion/sqllogictest/test_files/select.slt b/datafusion/sqllogictest/test_files/select.slt index 646a1db99e8ae..0b33a6b46d666 100644 --- a/datafusion/sqllogictest/test_files/select.slt +++ b/datafusion/sqllogictest/test_files/select.slt @@ -885,7 +885,6 @@ physical_plan SortExec: expr=[a@0 ASC NULLS LAST] --MemoryExec: partitions=1, partition_sizes=[1] - # run below query in multi partitions statement ok set datafusion.execution.target_partitions = 2; From 2f396672c4789d76229a1d26b2aff0709cb6f28f Mon Sep 17 00:00:00 2001 From: asura7969 <1402357969@qq.com> Date: Wed, 6 Dec 2023 23:04:32 +0800 Subject: [PATCH 8/9] add test --- datafusion/sqllogictest/test_files/select.slt | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/datafusion/sqllogictest/test_files/select.slt b/datafusion/sqllogictest/test_files/select.slt index 0b33a6b46d666..aefbb8b50ee5f 100644 --- a/datafusion/sqllogictest/test_files/select.slt +++ b/datafusion/sqllogictest/test_files/select.slt @@ -869,12 +869,6 @@ SELECT * EXCLUDE(d, b, c, a, a, b, c, d) FROM table1 # avoiding adding an alias if the column name is the same -query I -select a as a FROM table1 order by a ----- -1 -2 - query TT EXPLAIN select a as a FROM table1 order by a ---- @@ -885,6 +879,9 @@ physical_plan SortExec: expr=[a@0 ASC NULLS LAST] --MemoryExec: partitions=1, partition_sizes=[1] +query error DataFusion error: Schema error: Ambiguous reference to unqualified field a +EXPLAIN select a as a FROM table1 t1 CROSS JOIN table1 t2 order by a + # run below query in multi partitions statement ok set datafusion.execution.target_partitions = 2; From 0f65aca8d793f80fafefc52b07483591ca23876a Mon Sep 17 00:00:00 2001 From: asura7969 <1402357969@qq.com> Date: Wed, 6 Dec 2023 23:06:59 +0800 Subject: [PATCH 9/9] add test --- datafusion/sqllogictest/test_files/select.slt | 1 + 1 file changed, 1 insertion(+) diff --git a/datafusion/sqllogictest/test_files/select.slt b/datafusion/sqllogictest/test_files/select.slt index aefbb8b50ee5f..3f3befd85a592 100644 --- a/datafusion/sqllogictest/test_files/select.slt +++ b/datafusion/sqllogictest/test_files/select.slt @@ -879,6 +879,7 @@ physical_plan SortExec: expr=[a@0 ASC NULLS LAST] --MemoryExec: partitions=1, partition_sizes=[1] +# ambiguous column references in on join query error DataFusion error: Schema error: Ambiguous reference to unqualified field a EXPLAIN select a as a FROM table1 t1 CROSS JOIN table1 t2 order by a