Support tuples as types #11896

samuelcolvin · 2024-08-08T23:45:35Z

Which issue does this PR close?

Closes #6635, continues from #8590.

Rationale for this change

See #6635 — I want to be able to use tuples as per that issue

What changes are included in this PR?

Add tuples planning
add tuple coercion
tests

Are these changes tested?

yes.

Are there any user-facing changes?

More SQL supported.

(Fixed, ignore this)

~~This is mostly working and (a, b) in (('x', 'y')) is working correctly, however if you have more than one value in the lookup, e.g. ('x', 'y') IN (('x', 'y'), ('y', 'x')) an error occurs:~~

External error: query failed: DataFusion error: Arrow error: Invalid argument error: Nested comparison: Struct([Field { name: "c0", data_type: Utf8, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }, Field { name: "c1", data_type: Utf8, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }]) == Struct([Field { name: "c0", data_type: Utf8, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }, Field { name: "c1", data_type: Utf8, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }]) (hint: use make_comparator instead)
[SQL] select ('x', 'y') IN (('x', 'y'), ('y', 'x'));
at test_files/struct.slt:256

~~There are two tests that should fail.~~

samuelcolvin

I don't know why

datafusion/sqllogictest/test_files/struct.slt

jayzhan211 · 2024-08-09T01:46:21Z

DataFusion error: Arrow error: Invalid argument error: Nested comparison: Struct\(\[Field \{ name: "c0", data_type: Utf8, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: \{\} \}, Field \{ name: "c1", data_type: Utf8, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: \{\} \}\]\) == Struct\(\[Field \{ name: "c0", data_type: Utf8, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: \{\} \}, Field \{ name: "c1", data_type: Utf8, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: \{\} \}\]\) \(hint: use make_comparator instead\)

make_comparator is introduced recently for comparing nested type.

However, InListExpr is not able to compare with nested type.

fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
        let num_rows = batch.num_rows();
        let value = self.expr.evaluate(batch)?;
        let r = match &self.static_filter {
            Some(f) => f.contains(value.into_array(num_rows)?.as_ref(), self.negated)?,
            None => {
                let value = value.into_array(num_rows)?;
                let found = self.list.iter().map(|expr| expr.evaluate(batch)).try_fold(
                    BooleanArray::new(BooleanBuffer::new_unset(num_rows), None),
                    |result, expr| -> Result<BooleanArray> {
                        Ok(or_kleene(
                            &result,
                            &eq(&value, &expr?.into_array(num_rows)?)?,
                        )?)
                    },
                )?;

                if self.negated {
                    not(&found)?
                } else {
                    found
                }
            }
        };
        Ok(ColumnarValue::Array(Arc::new(r)))
    }

We need to change the eq to new function that calls compare_op_for_nested for nested type, and fallback to eq for non-nested type. I think there is similar issue left somewhere for other expressions, maybe we can have a general comparison function (nested + non-nested) and adapt it widely in datafusion

samuelcolvin · 2024-08-09T09:16:25Z

Thanks so much for the pointer @jayzhan211, this should now be fixed.

alamb

Thank you @samuelcolvin -- this looks very impressive to me. Beautifully simple

I had some small suggestions, but I don't think they are required to merge

Very nicely done

alamb · 2024-08-09T14:58:51Z

datafusion/expr/src/type_coercion/binary.rs

@@ -780,6 +781,31 @@ fn coerce_numeric_type_to_decimal256(numeric_type: &DataType) -> Option<DataType
    }
 }

+fn struct_coercion(lhs_type: &DataType, rhs_type: &DataType) -> Option<DataType> {


Could you please document what the intended rules for coercion are, in English, as a comment? That would help me understand if the code implements the intent correctly

I think the result is if all the types can be cocerced, then the output is a struct with fields named c1 , c2, etc

I wonder if it would be better / easier to understand if the names of the fields were preserved (aka the field names from lhs_fields)

alamb · 2024-08-09T15:01:30Z

datafusion/sql/src/expr/mod.rs

+                self.parse_struct(schema, planner_context, values, vec![])
+            }
+            None => not_impl_err!("Empty tuple not supported yet"),
+            _ => {


I was trying to figure out how to write a negative test for this case, however anything I could try seems to work

DataFusion CLI v41.0.0

create table foo as values (1);

> select * from foo where (column1) IN ((column1+1), (2)); +---------+ | column1 | +---------+ +---------+ 0 row(s) fetched. Elapsed 0.008 seconds. > select * from foo where (column1) IN ((column1+1), (2), (1)); +---------+ | column1 | +---------+ | 1 | +---------+ 1 row(s) fetched. Elapsed 0.010 seconds.

query error DataFusion error: This feature is not implemented: Only identifiers and literals are supported in tuples select a from values where (a, c) IN ((abs(a), 1));

Query success in duckdb

D select a from t where (a, b) in ((1, abs(a))); ┌────────┐ │ a │ │ int32 │ ├────────┤ │ 0 rows │ └────────┘

We could support abs() and other complex type in tuple in follow PR, but we should check not only first element of the values but all the values in this PR

I will file a ticket to track the feature

query error DataFusion error: This feature is not implemented: Only identifiers and literals are supported in tuples select a from values where (a, c) IN ((abs(a), 1));

Query success in duckdb

D select a from t where (a, b) in ((1, abs(a))); ┌────────┐ │ a │ │ int32 │ ├────────┤ │ 0 rows │ └────────┘

We could support abs() and other complex type in tuple in follow PR, but we should check not only first element of the values but all the values in this PR

I am not sure about whether our evaulation support struct array like {(struct(Expr1{column1}, 1)), struct (Expr2{column2}, 2))}?

alamb · 2024-08-09T15:03:34Z

datafusion/sqllogictest/test_files/struct.slt

+true
+
+query I
+select a from values where (a, c) = (1, 'a');


I recommend we also do a negative test, like

select a from values where (a, c) = (100, 'a');

alamb · 2024-08-09T15:04:20Z

FYI @my-vegetable-has-exploded

jayzhan211 · 2024-08-10T00:50:19Z

datafusion/expr/src/type_coercion/binary.rs

+                .into_iter()
+                .enumerate()
+                .map(|(i, datatype)| {
+                    Arc::new(Field::new(format!("c{i}"), datatype, true))


I think we should preserve field name and nullable, only type is updated

jayzhan211 · 2024-08-10T01:00:04Z

datafusion/sql/src/expr/mod.rs

+        planner_context: &mut PlannerContext,
+        values: Vec<SQLExpr>,
+    ) -> Result<Expr> {
+        match values.first() {


my-vegetable-has-exploded · 2024-08-11T02:48:50Z

Thanks @samuelcolvin. This is exactly what I wanted to do.

alamb · 2024-08-12T10:36:08Z

I took the liberty of merging up from main to resolve a conflict -- I think we could merge this PR and address the comments as a follow on PR, or we can do it prior to merging this one.

Please let me know what you prefer @samuelcolvin

alamb · 2024-08-12T19:02:48Z

Thanks again everyone 🚀

support tuples as types

96a3e18

github-actions bot added sql SQL Planner logical-expr Logical plan and expressions sqllogictest SQL Logic Tests (.slt) labels Aug 8, 2024

samuelcolvin commented Aug 8, 2024

View reviewed changes

datafusion/sqllogictest/test_files/struct.slt Show resolved Hide resolved

datafusion/sqllogictest/test_files/struct.slt Show resolved Hide resolved

use compare_op_for_nested

b46c645

github-actions bot added the physical-expr Physical Expressions label Aug 9, 2024

samuelcolvin marked this pull request as ready for review August 9, 2024 09:17

samuelcolvin changed the title ~~[HELP NEEDED] support tuples as types~~ Support tuples as types Aug 9, 2024

alamb approved these changes Aug 9, 2024

View reviewed changes

jayzhan211 reviewed Aug 10, 2024

View reviewed changes

Merge remote-tracking branch 'apache/main' into tuples

b45e617

github-actions bot removed the logical-expr Logical plan and expressions label Aug 12, 2024

alamb merged commit 140f7ce into apache:main Aug 12, 2024
24 checks passed

samuelcolvin deleted the tuples branch August 12, 2024 21:23

samuelcolvin mentioned this pull request Aug 29, 2024

We're changing database pydantic/logfire#408

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support tuples as types #11896

Support tuples as types #11896

samuelcolvin commented Aug 8, 2024 •

edited

Loading

samuelcolvin left a comment

jayzhan211 commented Aug 9, 2024 •

edited

Loading

samuelcolvin commented Aug 9, 2024

alamb left a comment

alamb Aug 9, 2024

alamb Aug 9, 2024

jayzhan211 Aug 10, 2024

alamb Aug 10, 2024

my-vegetable-has-exploded Aug 11, 2024

alamb Aug 9, 2024

alamb commented Aug 9, 2024

jayzhan211 Aug 10, 2024

jayzhan211 Aug 10, 2024

my-vegetable-has-exploded commented Aug 11, 2024

alamb commented Aug 12, 2024

alamb commented Aug 12, 2024

Support tuples as types #11896

Support tuples as types #11896

Conversation

samuelcolvin commented Aug 8, 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?

samuelcolvin left a comment

Choose a reason for hiding this comment

jayzhan211 commented Aug 9, 2024 • edited Loading

samuelcolvin commented Aug 9, 2024

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Aug 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

my-vegetable-has-exploded commented Aug 11, 2024

alamb commented Aug 12, 2024

alamb commented Aug 12, 2024

samuelcolvin commented Aug 8, 2024 •

edited

Loading

jayzhan211 commented Aug 9, 2024 •

edited

Loading