You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/04/25 09:16:13 UTC
[GitHub] [arrow-datafusion] houqp opened a new pull request #55: Support qualified columns in queries
houqp opened a new pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55
This turned out to be a much larger/destructive PR than I initially expected. Would like to get some early feedback on the approach before I spend more time working on the clean up. So far I have been able to get all unit tests to pass.
TODO:
- [ ] Address FIXMEs and TODOs
- [ ] Check integration tests
- [ ] Rebase to latest master
# Which issue does this PR close?
# What changes are included in this PR?
Here is the main design change introduced in this PR:
* Physical column expression now references columns by unique indices instead of names
* Logical column expression now wraps around a newly defined Column struct that can represent both qualified and unqualified columns
Query execution flow change:
1. When a TableScan plan has table_name set to `Some(name)`, all of the fields in its schema will be created as fully qualified fields.
1. Logical plan builder is responsible for normalizing all logical column expressions by adding qualifier based on schema wherever applicable.
1. Logical plan optimizer operates on normalized column expressions.
1. During physical planning, logical column expressions are resolved to physical column expressions with corresponding indices based on logical plan schemas. Notice a side effect of this is we don't look up column index during execution anymore. It is now done at planning time.
1. During physical planning, physical schema (arrow schema) has all column qualifiers stripped.
Some other changes introduced in this PR to help make all tests pass:
* avoid coalesce for hash repartition plan
* added partitioned hash join tests to hash_join module
* added support for join with alias (for self join)
* added join_using method to logical plan builder
* fixed couple other bugs here and there along the way, but couldn't remember :(
# Are there any user-facing changes?
breaking api changes:
* Column expression now wraps Column struct instead of String
* TableScan plan now takes table_name as Option instead of String
* Various dataframe scan method now takes table name as Option instead of &str
* Physical Column expression now requires index field
* logical builder join method now takes left and right keys as vector of Column structs
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on a change in pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#discussion_r651478410
##########
File path: ballista/rust/core/proto/ballista.proto
##########
@@ -408,6 +421,119 @@ message PhysicalPlanNode {
}
}
+// physical expressions
Review comment:
Yeah, i have the same feeling when it comes to debugging index offset issues :(
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-841614960
After a lot of back and forth with @jorgecarleitao, I have updated the invariant doc (https://docs.google.com/document/d/1dbK-3eaTHlzZcHzpTk1h-LA3b7dcxsVBcoZeVKYIPwI) to enforce strict equality in all optimization invariants. As a result, optimizations like reordering join sides need to add an extra projection to maintain the field order. We think the extra projection should have minimal overhead, but helps keep the field order semantic simple and easy for end users to understand.
This doc is ready for final review. In the mean time, I am going to work on fixing up the ballista builds.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on a change in pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#discussion_r619871814
##########
File path: datafusion/src/physical_optimizer/coalesce_batches.rs
##########
@@ -58,7 +59,17 @@ impl PhysicalOptimizerRule for CoalesceBatches {
// See https://issues.apache.org/jira/browse/ARROW-11068
let wrap_in_coalesce = plan_any.downcast_ref::<FilterExec>().is_some()
|| plan_any.downcast_ref::<HashJoinExec>().is_some()
- || plan_any.downcast_ref::<RepartitionExec>().is_some();
+ || {
+ match plan_any.downcast_ref::<RepartitionExec>() {
+ Some(p) => match p.partitioning() {
+ // do not coalesce hash partitions since other plans like partitioned hash
Review comment:
Good call, I started with handling this in `HashJoinStream`, but couldn't get it to work in all cases due to a small bug. I took a second look at my first attempt and noticed it could be fixed by changing 3 characters :P I have now reverted my change in the coalesce optimizer and added back my initial fix through https://github.com/apache/arrow-datafusion/pull/55/commits/696f8e04178fef877a658d0ef08c3c4ceebfcfdf.
Let me know if this fix aligns with what you have in mind. Also I suck at naming, `preserve_left` is the best I can come up with, suggestions welcome :)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
Dandandan commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-826301371
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-826380176
@jorgecarleitao your understanding on column changes in logical and physical planes are correct. I would add that in physical plane, the string column name is probably not needed. We are currently only using the index field for evaluation. I kept it mostly for debugging purpose. But given the column name info is also available in physical schema fields, I think it should be safe to only store index in physical column expressions.
The answer to your schema change question is a little bit tricky, let me try to clarify the new behavior. In short, it changes the field names in logical plan schemas because we require all columns to be normalized when building the plan. For physical schemas, there should be no change for column names except when columns are wrapped with operators.
Use your SQL as an example:
```sql
SELECT a FROM t1
```
The logical schema field will be normalized to `t1.a`. However, the final execution output will have a physical/arrow schema with field `a`. The qualifier is stripped during physical planning at: https://github.com/houqp/arrow-datafusion/blob/8ecc215bb7fe44d8cf9dcb4b90df753f0c50afb7/datafusion/src/physical_plan/planner.rs#L483-L486
For DataFrame API, the behavior is the same since both SQL and Dataframe go through the same query builder interface:
```rust
df = ctx.table("temp")?;
df.select("a").collect().schema().fields()[0].name()
```
The above code will result in `a`. So far this is the same as what datafusion does today. The difference comes in when operators are used, for example:
```sql
SELECT a, MAX(b) FROM t1
```
This will result in two unqualified fields `a` and `MAX(t1.b)`.
Basically I made sure the behavior is consistent with MySQL, Postgresql and Spark.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] andygrove commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-829296665
Thanks, @houqp! I'm excited to see this work. I have had a few attempts in the past at implementing some of these changes myself and they always ended up being hugely disruptive. I look forward to seeing the design document and will comment more once that is up.
One more thing we should think about is working with schemaless data sources. This isn't something we have really talked about yet and I have a concern about moving from resolving columns by name to using indices because it might limit our ability to support this use case in the future.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#discussion_r651125970
##########
File path: datafusion/src/physical_plan/planner.rs
##########
@@ -56,6 +56,121 @@ use expressions::col;
use log::debug;
use std::sync::Arc;
+fn create_function_physical_name(
+ fun: &str,
+ distinct: bool,
+ args: &[Expr],
+ input_schema: &DFSchema,
+) -> Result<String> {
+ let names: Vec<String> = args
+ .iter()
+ .map(|e| physical_name(e, input_schema))
+ .collect::<Result<_>>()?;
+
+ let distinct_str = match distinct {
+ true => "DISTINCT ",
+ false => "",
+ };
+ Ok(format!("{}({}{})", fun, distinct_str, names.join(",")))
+}
+
+fn physical_name(e: &Expr, input_schema: &DFSchema) -> Result<String> {
Review comment:
eventually maybe this might be easier to find if it were on `Expr` itself (e.g. `Expr::physical_name`)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-866215121
🚀
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] andygrove commented on a change in pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
andygrove commented on a change in pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#discussion_r619815966
##########
File path: datafusion/src/logical_plan/expr.rs
##########
@@ -27,14 +27,89 @@ use aggregates::{AccumulatorFunctionImplementation, StateTypeFunction};
use arrow::{compute::can_cast_types, datatypes::DataType};
use crate::error::{DataFusionError, Result};
-use crate::logical_plan::{DFField, DFSchema};
+use crate::logical_plan::{DFField, DFSchema, DFSchemaRef};
use crate::physical_plan::{
aggregates, expressions::binary_operator_data_type, functions, udf::ScalarUDF,
};
use crate::{physical_plan::udaf::AggregateUDF, scalar::ScalarValue};
use functions::{ReturnTypeFunction, ScalarFunctionImplementation, Signature};
use std::collections::HashSet;
+/// A named reference to a qualified filed in a schema.
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+pub struct Column {
+ /// relation/table name.
+ pub relation: Option<String>,
+ /// field/column name.
+ pub name: String,
Review comment:
Should we consider supporting compound names here so that we can eventually support reference nested types?
Perhaps Column could be an enum?
```rust
enum Column {
Simple(Option<String>, String),
Compound(Option<String>, Vec<String>)
}
```
##########
File path: datafusion/src/logical_plan/expr.rs
##########
@@ -27,14 +27,89 @@ use aggregates::{AccumulatorFunctionImplementation, StateTypeFunction};
use arrow::{compute::can_cast_types, datatypes::DataType};
use crate::error::{DataFusionError, Result};
-use crate::logical_plan::{DFField, DFSchema};
+use crate::logical_plan::{DFField, DFSchema, DFSchemaRef};
use crate::physical_plan::{
aggregates, expressions::binary_operator_data_type, functions, udf::ScalarUDF,
};
use crate::{physical_plan::udaf::AggregateUDF, scalar::ScalarValue};
use functions::{ReturnTypeFunction, ScalarFunctionImplementation, Signature};
use std::collections::HashSet;
+/// A named reference to a qualified filed in a schema.
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+pub struct Column {
+ /// relation/table name.
+ pub relation: Option<String>,
+ /// field/column name.
+ pub name: String,
Review comment:
Should we consider supporting compound names here so that we can eventually support nested types? For example `SELECT t1.customer.address.zip, ...`
Perhaps Column could be an enum?
```rust
enum Column {
Simple(Option<String>, String),
Compound(Option<String>, Vec<String>)
}
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-865151449
@alamb yes, this is ready to merge.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-827598513
FYI I added https://github.com/apache/arrow-datafusion/issues/211 to track "making the invariants document easier to find / ensure everyone is aligned"
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-832590731
> PostgreSQL won the laziest developer award.
LOL
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#discussion_r621203228
##########
File path: datafusion/src/logical_plan/plan.rs
##########
@@ -141,7 +137,7 @@ pub enum LogicalPlan {
/// Produces rows from a table provider by reference or from the context
TableScan {
/// The name of the table
- table_name: String,
+ table_name: Option<String>,
Review comment:
I don't fully understand how a table scan (which results in a relation) can end up without a name. What is an example of when we need to use a `None` table_name?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] andygrove edited a comment on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
andygrove edited a comment on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-830111572
> @andygrove could you give some examples for schemaless data sources? I would like to incorporate that into the design doc.
@houqp Sure. My limited experience with schemaless comes from Apache Drill. One use case is where you are reading JSON documents and there could be thousands of different attributes but each document only has a small subset. Rather than forcing every document into a fixed schema, you have a schema that contains one column named `*` or `**` (I can't quite remember how Drill does this). For each document (row) you can look up column by name and either get a value or null/None if the attribute doesn't exist on a particular row.
The benefit of this approach is that we don't need to create a Schema object with thousands of fields, and we don't need each RecordBatch to have thousands of empty arrays. Our current RecordBatch does not support this use case of course.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on a change in pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#discussion_r619792456
##########
File path: datafusion/src/execution/dataframe_impl.rs
##########
@@ -252,7 +252,12 @@ mod tests {
let right = test_table()?.select_columns(&["c1", "c3"])?;
let left_rows = left.collect().await?;
let right_rows = right.collect().await?;
- let join = left.join(right, JoinType::Inner, &["c1"], &["c1"])?;
+ let join = left.join(
+ right,
+ JoinType::Inner,
+ vec![Column::from_name("c1".to_string())],
Review comment:
I think it might be useful to keep supporting the string syntax to keep the dataframe api simple, but also support qualified names.
Maybe the API could be used `column: Vec<Into<Column>>` and includes an implementation for `&str`?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] andygrove commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-830111572
> @andygrove could you give some examples for schemaless data sources? I would like to incorporate that into the design doc.
@houqp Sure. My limited experience with schemaless comes from Apache Drill. One use case is where you are reading JSON documents and there could be thousands of different attributes but each document only has a small subset. Rather than forcing ever document into a fixed schema, you have a schema that contains one column named `*` or `**` (I can't quite remember how Drill does this). For each document (row) you can look up column by name and either get a value or null/None if the attribute doesn't exist on a particular row.
The benefit of this approach is that we don't need to create a Schema object with thousands of fields, and we don't need each RecordBatch to have thousands of empty arrays. Our current RecordBatch does not support this use case of course.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-832595330
FWIW I like the proposal in https://docs.google.com/document/d/1uviWavwEGD3qxwMk2AGkOgp6ENrvKGiMWQhHNbqPwhg/edit?usp=sharing for output name semantics. 👍
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#discussion_r622934472
##########
File path: datafusion/src/logical_plan/plan.rs
##########
@@ -141,7 +137,7 @@ pub enum LogicalPlan {
/// Produces rows from a table provider by reference or from the context
TableScan {
/// The name of the table
- table_name: String,
+ table_name: Option<String>,
Review comment:
I see -- thank you for the explanation. In this case in DataFusion, we could perhaps register the data with either a temporary name (or the csv file name or something).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] codecov-commenter edited a comment on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-855311829
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb edited a comment on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
alamb edited a comment on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-864939981
> So considering this bug also exits in the current master, I think it would be easier to merge the current reviewed PR as is. Then we can focus on my fix to discuss whether the change in join column handling logic is the right move or not. I am also not very happen with how my fix is implemented, so would love to get some ideas on alternative implementations as well.
I agree this is the best course of action.
Do you think this is ready to merge @houqp ?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on a change in pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#discussion_r622398455
##########
File path: datafusion/src/logical_plan/plan.rs
##########
@@ -141,7 +137,7 @@ pub enum LogicalPlan {
/// Produces rows from a table provider by reference or from the context
TableScan {
/// The name of the table
- table_name: String,
+ table_name: Option<String>,
Review comment:
In SQL this would never happen. It's more of a thing in spark land where for simple queries, people usually just load a csv or parquet partition into a dataframe without any table registration. So for example, for dataframe users, they could start with a `Context.read_csv` call and only reference columns by unqualified names in subsequent transformations.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-866221029
Thank you all for the reviews! When I initially started working on this, I thought it would only take me two weekends, lol. I will send the join semantic change diff as a follow up PR tonight.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp edited a comment on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp edited a comment on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-864515901
looks like integration test is failing due to mismatch between logical schema and physical schema for window queries, i will dig into this.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp edited a comment on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp edited a comment on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-826380176
@jorgecarleitao your understanding on column changes in logical and physical planes are correct. I would add that in physical plane, the string column name is probably not needed. We are currently only using the index field for evaluation. I kept it mostly for debugging purpose. But given the column name info is also available in physical schema fields, I think it should be safe to only store index in physical column expressions.
The answer to your schema change question is a little bit tricky, let me try to clarify the new behavior. In short, it changes the field names in logical plan schemas because we require all columns to be normalized when building the plan. For physical schemas, there should be no change for column names except when columns are wrapped with operators.
Using your SQL query as an example:
```sql
SELECT a FROM t1
```
The logical schema field will be normalized to `t1.a`. However, the final execution output will have a physical/arrow schema with a field `a`. The qualifier is stripped during physical planning at: https://github.com/houqp/arrow-datafusion/blob/8ecc215bb7fe44d8cf9dcb4b90df753f0c50afb7/datafusion/src/physical_plan/planner.rs#L483-L486
For DataFrame API, the behavior is the same since both SQL and Dataframe go through the same query builder interface:
```rust
df = ctx.table("temp")?;
df.select("a").collect().schema().fields()[0].name()
```
The above code will result in `a`. So far this is the same as what datafusion does today. The difference comes in when operators are involed, for example:
```sql
SELECT a, MAX(b) FROM t1
```
This will result in two unqualified fields `a` and `MAX(t1.b)`. Here are some more related sample outputs in the tests:
* https://github.com/houqp/arrow-datafusion/blob/8ecc215bb7fe44d8cf9dcb4b90df753f0c50afb7/datafusion/src/execution/context.rs#L1629-L1637
* https://github.com/houqp/arrow-datafusion/blob/8ecc215bb7fe44d8cf9dcb4b90df753f0c50afb7/datafusion/src/execution/context.rs#L1680-L1704
* https://github.com/houqp/arrow-datafusion/blob/8ecc215bb7fe44d8cf9dcb4b90df753f0c50afb7/datafusion/src/execution/context.rs#L1832-L1838
* https://github.com/houqp/arrow-datafusion/blob/8ecc215bb7fe44d8cf9dcb4b90df753f0c50afb7/datafusion/src/execution/context.rs#L2313-L2322
Basically I made sure the behavior is consistent with MySQL, Postgresql and Spark.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] jorgecarleitao commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-828928481
@houqp , let me clarify the context of the invariants: back in August 2020, I started looking into improving the schema invariants, as DataFusion was not preserving any. Before committing to any work, I outlined the invariants that I though were relevant, shared it around, and worked to enforce them. Please by no means consider it to be like something that "the community" agreed; it was more like a guideline for me, which at the time some people read it through. I felt the need to have some design choices to guide my development at the time to arrive at a consistent state.
Wrt to your question, I think that you are right: taking the assumption `column names are unique`, it follows that the order of columns in a schema does not need to be preserved because it is expected that consumers access columns by name, not by index. So, I think that the `==` in `schema(plan) == schema(opt(plan))` should represent the same metadata, set (in a mathematical sense) of fields. We may want to define this equality somewhere in DataFusion so that we can use it whenever we want to assert the invariant (since the compiler can't do it for us).
I also agree that we could also preserve column order: in the case of the join, we would need to make the side probe optimization to be a physical, not logical, optimization (and write the batches with the correct order at the end), and in `*` we would define a rule for column order.
I do not have an opinion in either direction; I just think that it is useful to be explicit about these so that our users can have a mental model on how column names behave and how they access them (can I rely on stable column names?, can I rely on stable indexes?, etc.).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-831405269
fixed permission for google doc, it was setup to be read only :(
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#discussion_r651112853
##########
File path: datafusion/src/logical_plan/dfschema.rs
##########
@@ -208,6 +297,28 @@ impl Into<Schema> for DFSchema {
}
}
+impl Into<Schema> for &DFSchema {
+ /// Convert a schema into a DFSchema
+ fn into(self) -> Schema {
+ Schema::new(
+ self.fields
+ .iter()
+ .map(|f| {
+ if f.qualifier().is_some() {
Review comment:
its almost like this could be `f.feld.clone() even if `f.qualifier()` is Some
##########
File path: datafusion/src/optimizer/projection_push_down.rs
##########
@@ -417,15 +492,52 @@ mod tests {
.aggregate(vec![], vec![max(col("b"))])?
.build()?;
- let expected = "Aggregate: groupBy=[[]], aggr=[[MAX(#b)]]\
- \n Filter: #c\
+ let expected = "Aggregate: groupBy=[[]], aggr=[[MAX(#test.b)]]\
+ \n Filter: #test.c\
\n TableScan: test projection=Some([1, 2])";
assert_optimized_plan_eq(&plan, expected);
Ok(())
}
+ #[test]
+ fn join_schema_trim() -> Result<()> {
+ let table_scan = test_table_scan()?;
+
+ let schema = Schema::new(vec![Field::new("c1", DataType::UInt32, false)]);
+ let table2_scan =
+ LogicalPlanBuilder::scan_empty(Some("test2"), &schema, None)?.build()?;
+
+ let plan = LogicalPlanBuilder::from(&table_scan)
+ .join(&table2_scan, JoinType::Left, vec!["a"], vec!["c1"])?
+ .project(vec![col("a"), col("b"), col("c1")])?
+ .build()?;
+
+ // make sure projections are pushed down to table scan
+ let expected = "Projection: #test.a, #test.b, #test2.c1\
Review comment:
👍
##########
File path: datafusion/src/logical_plan/plan.rs
##########
@@ -125,9 +130,11 @@ pub enum LogicalPlan {
/// Right input
right: Arc<LogicalPlan>,
/// Equijoin clause expressed as pairs of (left, right) join columns
- on: Vec<(String, String)>,
+ on: Vec<(Column, Column)>,
Review comment:
👍
##########
File path: datafusion/src/logical_plan/expr.rs
##########
@@ -33,6 +33,90 @@ use std::collections::HashSet;
use std::fmt;
use std::sync::Arc;
+/// A named reference to a qualified filed in a schema.
Review comment:
```suggestion
/// A named reference to a qualified field in a schema.
```
##########
File path: ballista/rust/core/proto/ballista.proto
##########
@@ -408,6 +421,119 @@ message PhysicalPlanNode {
}
}
+// physical expressions
Review comment:
I think this is a very common tactic in other systems and so 👍
My only regret (?) is that I have spent non trivial amounts of time tracking down bugs when the offsets get messed up -- lol! But I don't have any better suggestions
##########
File path: datafusion/src/logical_plan/expr.rs
##########
@@ -27,14 +27,89 @@ use aggregates::{AccumulatorFunctionImplementation, StateTypeFunction};
use arrow::{compute::can_cast_types, datatypes::DataType};
use crate::error::{DataFusionError, Result};
-use crate::logical_plan::{DFField, DFSchema};
+use crate::logical_plan::{DFField, DFSchema, DFSchemaRef};
use crate::physical_plan::{
aggregates, expressions::binary_operator_data_type, functions, udf::ScalarUDF,
};
use crate::{physical_plan::udaf::AggregateUDF, scalar::ScalarValue};
use functions::{ReturnTypeFunction, ScalarFunctionImplementation, Signature};
use std::collections::HashSet;
+/// A named reference to a qualified filed in a schema.
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+pub struct Column {
+ /// relation/table name.
+ pub relation: Option<String>,
+ /// field/column name.
+ pub name: String,
Review comment:
I suggest we track this as a follow on ticket
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on a change in pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#discussion_r619873424
##########
File path: datafusion/src/physical_plan/hash_join.rs
##########
@@ -891,6 +898,36 @@ impl Stream for HashJoinStream {
}
Some(result)
}
+ // If maybe_batch is None and num_output_rows is 0, that means right side batch was
+ // empty and has been coalesced to None. Fill right side with Null if preserve_left
+ // is true.
+ None if self.preserve_left && self.num_output_rows == 0 => {
Review comment:
I think this partially resolves a more general issue with the left join, which is that it doesn't keep track of unmatched left rows across batches. https://issues.apache.org/jira/browse/ARROW-10971
Maybe we can add a TODO here / issue that we should generalize this to produce rows that were not matched.
This looks like a great start for that 👍
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp edited a comment on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp edited a comment on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-826380176
@jorgecarleitao your understanding on column changes in logical and physical planes are correct. I would add that in physical plane, the string column name is probably not needed. We are currently only using the index field for evaluation. I kept it mostly for debugging purpose. But given the column name info is also available in physical schema fields, I think it should be safe to only store index in physical column expressions.
The answer to your schema change question is a little bit tricky, let me try to clarify the new behavior. In short, it changes the field names in logical plan schemas because we require all columns to be normalized when building the plan. For physical schemas, there should be no change for column names except when columns are wrapped with operators.
Using your SQL query as an example:
```sql
SELECT a FROM t1
```
The logical schema field will be normalized to `t1.a`. However, the final execution output will have a physical/arrow schema with a field `a`. The qualifier is stripped during physical planning at: https://github.com/houqp/arrow-datafusion/blob/8ecc215bb7fe44d8cf9dcb4b90df753f0c50afb7/datafusion/src/physical_plan/planner.rs#L483-L486
For DataFrame API, the behavior is the same since both SQL and Dataframe go through the same query builder interface:
```rust
df = ctx.table("temp")?;
df.select("a").collect().schema().fields()[0].name()
```
The above code will result in `a`. So far this is the same as what datafusion does today. The difference comes in when operators are involed, for example:
```sql
SELECT a, MAX(b) FROM t1
```
This will result in two unqualified fields `a` and `MAX(t1.b)`.
Basically I made sure the behavior is consistent with MySQL, Postgresql and Spark.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb edited a comment on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
alamb edited a comment on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-832595330
Thanks @houqp ! FWIW I like the proposal in https://docs.google.com/document/d/1uviWavwEGD3qxwMk2AGkOgp6ENrvKGiMWQhHNbqPwhg/edit?usp=sharing for output name semantics. 👍
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-866238181
I filed a ticket to track some follow on work: https://github.com/apache/arrow-datafusion/issues/601
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on a change in pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#discussion_r624680122
##########
File path: datafusion/src/physical_plan/hash_join.rs
##########
@@ -891,6 +898,36 @@ impl Stream for HashJoinStream {
}
Some(result)
}
+ // If maybe_batch is None and num_output_rows is 0, that means right side batch was
+ // empty and has been coalesced to None. Fill right side with Null if preserve_left
+ // is true.
+ None if self.preserve_left && self.num_output_rows == 0 => {
Review comment:
FYI @houqp I added a more general fix here that deals with all unmatched left rows, so I think you could rebase if that change is merged.
https://github.com/apache/arrow-datafusion/pull/238
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on a change in pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#discussion_r619864093
##########
File path: datafusion/src/execution/dataframe_impl.rs
##########
@@ -252,7 +252,12 @@ mod tests {
let right = test_table()?.select_columns(&["c1", "c3"])?;
let left_rows = left.collect().await?;
let right_rows = right.collect().await?;
- let join = left.join(right, JoinType::Inner, &["c1"], &["c1"])?;
+ let join = left.join(
+ right,
+ JoinType::Inner,
+ vec![Column::from_name("c1".to_string())],
Review comment:
That's a great idea!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp edited a comment on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp edited a comment on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-826380176
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Jimexist commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
Jimexist commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-864556916
> looks like integration test is failing due to mismatch between logical schema and physical schema for window queries, i will dig into this.
thanks, it might be because of some edge cases that i did miss. i didn't find time to fix it but so far this query will fail:
```sql
select max(c1) over () max_c1, max(c1) over (partition by c2) max_c1_by_c2 from test;
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-835990383
@seddonm1 good news is we are already doing better than postgresql :) What we have right now is basically what Spark has. I can see us making this configurable in the future if there is a legit use-case for being fully compatible with postgres, mysql etc.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] andygrove commented on a change in pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
andygrove commented on a change in pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#discussion_r619815966
##########
File path: datafusion/src/logical_plan/expr.rs
##########
@@ -27,14 +27,89 @@ use aggregates::{AccumulatorFunctionImplementation, StateTypeFunction};
use arrow::{compute::can_cast_types, datatypes::DataType};
use crate::error::{DataFusionError, Result};
-use crate::logical_plan::{DFField, DFSchema};
+use crate::logical_plan::{DFField, DFSchema, DFSchemaRef};
use crate::physical_plan::{
aggregates, expressions::binary_operator_data_type, functions, udf::ScalarUDF,
};
use crate::{physical_plan::udaf::AggregateUDF, scalar::ScalarValue};
use functions::{ReturnTypeFunction, ScalarFunctionImplementation, Signature};
use std::collections::HashSet;
+/// A named reference to a qualified filed in a schema.
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+pub struct Column {
+ /// relation/table name.
+ pub relation: Option<String>,
+ /// field/column name.
+ pub name: String,
Review comment:
Should we consider supporting compound names here so that we can eventually support nested types? For example `SELECT t1.customer.address.zip, ...`
Perhaps Column could be an enum?
```rust
enum Column {
Simple(Option<String>, String),
Compound(Option<String>, Vec<String>)
}
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] jorgecarleitao commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-827755259
So, having done this type of exercises before in DataFusion, I tend to agree that these things are really difficult to divide in smaller PRs, as they touch delicate balances in the code.
I agree that the current way we handle names has reached its limits, and thus I am super happy to see this PR.
wrt to the invariants, I do think that we should be able to address them here. I am still trying to understand why we need to break invariants to enable qualified column names. This is the gist of my comment: are these changes by design, a theoretical impediment whereby we can't have both qualified names and the invariants, or derived from this still being work in progress?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on a change in pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#discussion_r619792456
##########
File path: datafusion/src/execution/dataframe_impl.rs
##########
@@ -252,7 +252,12 @@ mod tests {
let right = test_table()?.select_columns(&["c1", "c3"])?;
let left_rows = left.collect().await?;
let right_rows = right.collect().await?;
- let join = left.join(right, JoinType::Inner, &["c1"], &["c1"])?;
+ let join = left.join(
+ right,
+ JoinType::Inner,
+ vec![Column::from_name("c1".to_string())],
Review comment:
I think it might be useful to keep supporting the string syntax to keep the dataframe api simple, but also support qualified references.
Maybe the API could be used `column: Vec<Into<Column>>` and includes an implementation for `&str`?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb edited a comment on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
alamb edited a comment on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-864939981
> So considering this bug also exits in the current master, I think it would be easier to merge the current reviewed PR as is. Then we can focus on my fix to discuss whether the change in join column handling logic is the right move or not. I am also not very happen with how my fix is implemented, so would love to get some ideas on alternative implementations as well.
I agree this is the best course of action.
Do you think this is ready to merge @houqp ?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on a change in pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#discussion_r632895922
##########
File path: datafusion/src/logical_plan/plan.rs
##########
@@ -141,7 +137,7 @@ pub enum LogicalPlan {
/// Produces rows from a table provider by reference or from the context
TableScan {
/// The name of the table
- table_name: String,
+ table_name: Option<String>,
Review comment:
Updated code to assign `?table?` as name for unnamed tables. TableScan name is not an Option anymore.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-860150632
@andygrove @Dandandan @jorgecarleitao @alamb @Jimexist this PR is now ready for review. We are now able to pass both tpch-7 and tpch-8.
I filed https://github.com/apache/arrow-datafusion/issues/549 to track compound column support as a fast follow task since this PR is already very large and keeps growing every time I rebase on top of the latest master. If you all think I should include compound column support in this PR, I am more than happy to add 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#discussion_r651124509
##########
File path: datafusion/src/physical_plan/expressions/nth_value.rs
##########
@@ -195,7 +195,7 @@ mod tests {
fn first_value() -> Result<()> {
let first_value = Arc::new(NthValue::first_value(
"first_value".to_owned(),
- col("arr"),
+ Arc::new(Column::new("arr", 0)),
Review comment:
FYI @Jimexist
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-864615921
@alamb, turns out the bug you demoed is not simple to fix and surfaced a problem in how join columns are handled in the current design. Based on what I have seen in MySQL and PostgreSQL, join columns deduplication should only be applied to join clause with `USING` constraints. We are currently applying the deduplication for all join types other than semi/anti joins.
I have a fix in https://github.com/houqp/arrow-datafusion/commit/76367875b103c3a8274ca6e8f660aa1c6177b668, which touched a lot of files and is a 600+ LoC diff in itself.
So considering this bug also exits in the current master, I think it would be easier to merge the current reviewed PR as is. Then we can focus on my fix to discuss whether the change in join column handling logic is the right move or not. I am also not very happen with how my fix is implemented, so would love to get some ideas on alternative implementations as well.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-864939981
> So considering this bug also exits in the current master, I think it would be easier to merge the current reviewed PR as is. Then we can focus on my fix to discuss whether the change in join column handling logic is the right move or not. I am also not very happen with how my fix is implemented, so would love to get some ideas on alternative implementations as well.
I agree this is the best course of action.
Do you think this is ready to merge @houqp
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
Dandandan commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-826301371
This is really cool @houqp thanks for doing this!
It's looking good to me, I added some thoughts
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-864939981
> So considering this bug also exits in the current master, I think it would be easier to merge the current reviewed PR as is. Then we can focus on my fix to discuss whether the change in join column handling logic is the right move or not. I am also not very happen with how my fix is implemented, so would love to get some ideas on alternative implementations as well.
I agree this is the best course of action.
Do you think this is ready to merge @houqp
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] andygrove commented on a change in pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
andygrove commented on a change in pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#discussion_r619815966
##########
File path: datafusion/src/logical_plan/expr.rs
##########
@@ -27,14 +27,89 @@ use aggregates::{AccumulatorFunctionImplementation, StateTypeFunction};
use arrow::{compute::can_cast_types, datatypes::DataType};
use crate::error::{DataFusionError, Result};
-use crate::logical_plan::{DFField, DFSchema};
+use crate::logical_plan::{DFField, DFSchema, DFSchemaRef};
use crate::physical_plan::{
aggregates, expressions::binary_operator_data_type, functions, udf::ScalarUDF,
};
use crate::{physical_plan::udaf::AggregateUDF, scalar::ScalarValue};
use functions::{ReturnTypeFunction, ScalarFunctionImplementation, Signature};
use std::collections::HashSet;
+/// A named reference to a qualified filed in a schema.
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+pub struct Column {
+ /// relation/table name.
+ pub relation: Option<String>,
+ /// field/column name.
+ pub name: String,
Review comment:
Should we consider supporting compound names here so that we can eventually support reference nested types?
Perhaps Column could be an enum?
```rust
enum Column {
Simple(Option<String>, String),
Compound(Option<String>, Vec<String>)
}
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp edited a comment on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp edited a comment on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-828880763
@jorgecarleitao looking more into the logical optimization invariants, I think we might want to relax it a little bit to account for some optimizations that may change column orders: https://github.com/apache/arrow-datafusion/blob/57eeb64659b9ca9c496a959f7716090fb32085b6/datafusion/src/optimizer/hash_build_probe_order.rs#L122-L133.
So basically something like this:
* If projection plan is the root node, then we can guarantee strict schema invariants for logical optimization, i.e. we preserve the exact same schema field vector.
* If root node is not a projection plan, we only guarantee same set of schema fields to be preserved, but not the order
From the user's point of view, it also makes sense since if I am executing a query like `SELECT * FROM t`, I am basically saying just give me all the columns in whatever order.
Technically, we could still enforce strict schema invariants for all plans by manually wrapping a projection plan when the outer plan is not a projection. But I think this adds unnecessary execution overhead for minor semantic gain.
Interesting in what others think about this.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp edited a comment on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp edited a comment on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-826380176
@jorgecarleitao your understanding on column changes in logical and physical planes are correct. I would add that in physical plane, the string column name is probably not needed. We are currently only using the index field for evaluation. I kept it mostly for debugging purpose. But given the column name info is also available in physical schema fields, I think it should be safe to only store index in physical column expressions.
The answer to your schema change question is a little bit tricky, let me try to clarify the new behavior. In short, it changes the field names in logical plan schemas because we require all columns to be normalized when building the plan. For physical schemas, there should be no change for column names except when columns are wrapped with operators.
Using your SQL query as an example:
```sql
SELECT a FROM t1
```
The logical schema field will be normalized to `t1.a`. However, the final execution output will have a physical/arrow schema with field `a`. The qualifier is stripped during physical planning at: https://github.com/houqp/arrow-datafusion/blob/8ecc215bb7fe44d8cf9dcb4b90df753f0c50afb7/datafusion/src/physical_plan/planner.rs#L483-L486
For DataFrame API, the behavior is the same since both SQL and Dataframe go through the same query builder interface:
```rust
df = ctx.table("temp")?;
df.select("a").collect().schema().fields()[0].name()
```
The above code will result in `a`. So far this is the same as what datafusion does today. The difference comes in when operators are used, for example:
```sql
SELECT a, MAX(b) FROM t1
```
This will result in two unqualified fields `a` and `MAX(t1.b)`.
Basically I made sure the behavior is consistent with MySQL, Postgresql and Spark.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#discussion_r622169779
##########
File path: datafusion/src/logical_plan/plan.rs
##########
@@ -141,7 +137,7 @@ pub enum LogicalPlan {
/// Produces rows from a table provider by reference or from the context
TableScan {
/// The name of the table
- table_name: String,
+ table_name: Option<String>,
Review comment:
I guess I don't fully understand how we could be scanning relations that don't have names (like how do you refer to them in SQL?) As I recall, postgres doesn't use the term 'table name' and instead uses the word 'relation name' as the name can come from a table, or an alias, or subquery (and probably others I am not remembering now)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-826380176
@jorgecarleitao your understanding on column changes in logical and physical planes are correct. I would add that in physical plane, the string column name is probably not needed. We are currently only using the index field for evaluation. I kept it mostly for debugging purpose. But given the column name info is also available in physical schema fields, I think it should be safe to only store index in physical column expressions.
The answer to your schema change question is a little bit tricky, let me try to clarify the new behavior. In short, it changes the field names in logical plan schemas because we require all columns to be normalized when building the plan. For physical schemas, there should be no change for column names except when columns are wrapped with operators.
Use your SQL as an example:
```sql
SELECT a FROM t1
```
The logical schema field will be normalized to `t1.a`. However, the final execution output will have a physical/arrow schema with field `a`. The qualifier is stripped during physical planning at: https://github.com/houqp/arrow-datafusion/blob/8ecc215bb7fe44d8cf9dcb4b90df753f0c50afb7/datafusion/src/physical_plan/planner.rs#L483-L486
For DataFrame API, the behavior is the same since both SQL and Dataframe go through the same query builder interface:
```rust
df = ctx.table("temp")?;
df.select("a").collect().schema().fields()[0].name()
```
The above code will result in `a`. So far this is the same as what datafusion does today. The difference comes in when operators are used, for example:
```sql
SELECT a, MAX(b) FROM t1
```
This will result in two unqualified fields `a` and `MAX(t1.b)`.
Basically I made sure the behavior is consistent with MySQL, Postgresql and Spark.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-866222968
Thanks for the epic work @houqp -- this is really great to see
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-860188184
This looks cool @houqp -- I plan to review this carefully tomorrow
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Jimexist edited a comment on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
Jimexist edited a comment on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-864556916
> looks like integration test is failing due to mismatch between logical schema and physical schema for window queries, i will dig into this.
thanks, it might be because of some edge cases that i did miss. i didn't find time to fix it but so far this query will fail:
```sql
> select max(c1) over () max_c1, max(c1) over (partition by c2) max_c1_by_c2 from test;
Plan("Schema contains duplicate unqualified field name 'MAX(c1)'")
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-842637864
> This doc is ready for final review. In the mean time, I am going to work on fixing up the ballista builds.
I wonder if it would be helpful to send a note to the dev mailing list asking for feedback on the doc
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
Dandandan commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-826340939
I think as part of this PR we can also have a look whether query 7/8 of the tpch benchmark are succeeding and add them to the regression test if they do :)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
Dandandan commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-866217530
Woohoo 🙌🙌🙌🙌 🎉 🎉🎉🎉
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on a change in pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#discussion_r651479583
##########
File path: datafusion/src/logical_plan/dfschema.rs
##########
@@ -208,6 +297,28 @@ impl Into<Schema> for DFSchema {
}
}
+impl Into<Schema> for &DFSchema {
+ /// Convert a schema into a DFSchema
+ fn into(self) -> Schema {
+ Schema::new(
+ self.fields
+ .iter()
+ .map(|f| {
+ if f.qualifier().is_some() {
Review comment:
good call, you are right.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp edited a comment on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp edited a comment on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-864615921
@alamb, turns out the bug you demoed is not simple to fix and surfaced a problem in how join columns are handled in the current design. Based on what I have seen in MySQL and PostgreSQL, join columns deduplication should only be applied to join clauses with `USING` constraints. We are currently applying the deduplication for all join types other than semi/anti joins.
I have a fix in https://github.com/houqp/arrow-datafusion/commit/76367875b103c3a8274ca6e8f660aa1c6177b668, which touched a lot of files and is a 600+ LoC diff in itself.
So considering this bug also exits in the current master, I think it would be easier to merge the current reviewed PR as is. Then we can focus on my fix to discuss whether the change in join column handling logic is the right move or not. I am also not very happen with how my fix is implemented, so would love to get some ideas on alternative implementations as well.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on a change in pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#discussion_r619791415
##########
File path: datafusion/src/physical_optimizer/coalesce_batches.rs
##########
@@ -58,7 +59,17 @@ impl PhysicalOptimizerRule for CoalesceBatches {
// See https://issues.apache.org/jira/browse/ARROW-11068
let wrap_in_coalesce = plan_any.downcast_ref::<FilterExec>().is_some()
|| plan_any.downcast_ref::<HashJoinExec>().is_some()
- || plan_any.downcast_ref::<RepartitionExec>().is_some();
+ || {
+ match plan_any.downcast_ref::<RepartitionExec>() {
+ Some(p) => match p.partitioning() {
+ // do not coalesce hash partitions since other plans like partitioned hash
+ // join depends on it empty batches for outter joins
Review comment:
```suggestion
// join depends on it empty batches for outer joins
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on a change in pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#discussion_r619891670
##########
File path: datafusion/src/execution/dataframe_impl.rs
##########
@@ -252,7 +252,12 @@ mod tests {
let right = test_table()?.select_columns(&["c1", "c3"])?;
let left_rows = left.collect().await?;
let right_rows = right.collect().await?;
- let join = left.join(right, JoinType::Inner, &["c1"], &["c1"])?;
+ let join = left.join(
+ right,
+ JoinType::Inner,
+ vec![Column::from_name("c1".to_string())],
Review comment:
Turns out it is not possible to use `Vec<impl Into<Column>>` in the signature because Dataframe is used as trait object. I changed the signature back to `&[&str]` for dataframe API and switched to `Vec<impl Into<Column>>` in the logical plan builder to keep the high level API simple.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp edited a comment on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp edited a comment on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-826380176
@jorgecarleitao your understanding on column changes in logical and physical planes are correct. I would add that in physical plane, the string column name is probably not needed. We are currently only using the index field for evaluation. I kept it mostly for debugging purpose. But given the column name info is also available in physical schema fields, I think it should be safe to only store index in physical column expressions.
The answer to your schema change question is a little bit tricky, let me try to clarify the new behavior. In short, it changes the field names in logical plan schemas because we require all columns to be normalized when building the plan. For physical schemas, there should be no change for column names except when columns are wrapped with operators.
Using your SQL query as an example:
```sql
SELECT a FROM t1
```
The logical schema field will be normalized to `t1.a`. However, the final execution output will have a physical/arrow schema with a field `a`. The qualifier is stripped during physical planning at: https://github.com/houqp/arrow-datafusion/blob/8ecc215bb7fe44d8cf9dcb4b90df753f0c50afb7/datafusion/src/physical_plan/planner.rs#L483-L486
For DataFrame API, the behavior is the same since both SQL and Dataframe go through the same query builder interface:
```rust
df = ctx.table("temp")?;
df.select("a").collect().schema().fields()[0].name()
```
The above code will result in `a`. So far this is the same as what datafusion does today. The difference comes in when operators are used, for example:
```sql
SELECT a, MAX(b) FROM t1
```
This will result in two unqualified fields `a` and `MAX(t1.b)`.
Basically I made sure the behavior is consistent with MySQL, Postgresql and Spark.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#discussion_r622169779
##########
File path: datafusion/src/logical_plan/plan.rs
##########
@@ -141,7 +137,7 @@ pub enum LogicalPlan {
/// Produces rows from a table provider by reference or from the context
TableScan {
/// The name of the table
- table_name: String,
+ table_name: Option<String>,
Review comment:
I guess I don't fully understand how we could be scanning relations that don't have names (like how do you refer to them in SQL?)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Jimexist commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
Jimexist commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-864557072
created https://github.com/apache/arrow-datafusion/issues/592 to address this
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-864608955
after digging more into this, the bug seems to be related to the projection push down optimizer, we are breaking the [Logical schema is invariant under logical optimization](https://github.com/apache/arrow-datafusion/blob/master/docs/specification/invariants.md#logical-schema-is-invariant-under-logical-optimization) invariant for both window and aggregate plans. This issue has been in our code base for awhile, but only surfaced through the newly added window order by integration test.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-865151449
@alamb yes, this is ready to merge.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] jorgecarleitao commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-866228974
Thank you @houqp for this excellent PR, together with the .md docs. They set a really important foundation to the query's semantics 💯
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp edited a comment on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp edited a comment on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-826380176
@jorgecarleitao your understanding on column changes in logical and physical planes are correct. I would add that in physical plane, the string column name is probably not needed. We are currently only using the index field for evaluation. I kept it mostly for debugging purpose. But given the column name info is also available in physical schema fields, I think it should be safe to only store index in physical column expressions.
The answer to your schema change question is a little bit tricky, let me try to clarify the new behavior. In short, it changes the field names in logical plan schemas because we require all columns to be normalized when building the plan. For physical schemas, there should be no change for column names except when columns are wrapped with operators.
Using your SQL query as an example:
```sql
SELECT a FROM t1
```
The logical schema field will be normalized to `t1.a`. However, the final execution output will have a physical/arrow schema with a field `a`. The qualifier is stripped during physical planning at: https://github.com/houqp/arrow-datafusion/blob/8ecc215bb7fe44d8cf9dcb4b90df753f0c50afb7/datafusion/src/physical_plan/planner.rs#L483-L486
For DataFrame API, the behavior is the same since both SQL and Dataframe go through the same query builder interface:
```rust
df = ctx.table("temp")?;
df.select("a").collect().schema().fields()[0].name()
```
The above code will result in `a`. So far this is the same as what datafusion does today. The difference comes in when operators are involed, for example:
```sql
SELECT a, MAX(b) FROM t1
```
This will result in two unqualified fields `a` and `MAX(t1.b)`. Here are some more related sample outputs in the tests:
* https://github.com/houqp/arrow-datafusion/blob/8ecc215bb7fe44d8cf9dcb4b90df753f0c50afb7/datafusion/src/execution/context.rs#L1629-L1637
* https://github.com/houqp/arrow-datafusion/blob/8ecc215bb7fe44d8cf9dcb4b90df753f0c50afb7/datafusion/src/execution/context.rs#L1680-L1704
Basically I made sure the behavior is consistent with MySQL, Postgresql and Spark.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-827766515
Thanks @jorgecarleitao for the invariant design doc, I wasn't aware of it before. I will definitely go through it and write something up for this PR this weekend. I think we should spend some effort into adding that doc into this repo as well. This is the kind of reference I have wanted to have while working on the patch set.
My first attempt of this does preserve the invariant of `SELECT t.a FROM t must result in the schema field name t.a`, but I later found out this would cause issue with subsequent read from writes. Basically the reader will end up with something like `t.t.a`. So I changed the behavior to be consistent with Spark and MySQL.
For breaking changes in schema fields involving operators, I will do more research on my end. The current behavior is the one with the least amount of work for me. But I am leaning towards better compatibility with the current data eng ecosystem like spark, mysql and postgres.
I fully agree with the optimization invariants. I have been thinking about whether there is better way to enforce them more formally other than unit tests.
@jorgecarleitao to your last question, all the invariants, other than the first one I mentioned in this comment, are pretty straight forward to maintain by tracking the user provided input in the column struct. I didn't do it because I wasn't aware of these invariants.
@alamb I am with you for the size of this PR as well :( That's why I decided to send it out early before finishing it because I noticed it just kept growing in size especially after every time I merge with master :P. I will try to see if I can restrict the change to just logical plans, but as @jorgecarleitao mentioned it might not be easy. I will do it if the overhead turned out to be small.
Anyway, putting code change aside, I think the most urgent thing to do is for me to write up a design doc to fully spec out the semantics so we are all on the same page on what needs to be implemented.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] seddonm1 commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
seddonm1 commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-832602918
Great work @houqp
I would be inclined to just copy the Postgres approach given it supports most of the complex use cases - even if it is a bit lazy 😃
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-831115015
Alright, I wrote some docs over the weekend to help align expectations:
* Document for output schema field name semantics with examples: https://docs.google.com/document/d/1uviWavwEGD3qxwMk2AGkOgp6ENrvKGiMWQhHNbqPwhg/edit?usp=sharing
* Proposed change to @jorgecarleitao 's invariant doc: https://docs.google.com/document/d/158gbfDp8pcakfriT2l7dHChwJB43_RV7lcWfxEC73ng/edit?usp=sharing
* Updated invariant doc with proposed changes applied: https://docs.google.com/document/d/1dbK-3eaTHlzZcHzpTk1h-LA3b7dcxsVBcoZeVKYIPwI/edit?usp=sharing
Please feel free to comment and make suggestions in the docs. One conclusion that came out of my research is everyone is naming output fields in a slightly different way and PostgreSQL wins the laziest developer award.
@andygrove with regards to schemaless I am thinking it might be better to handle them with a new set of schemaless physical nodes. All of our current physical nodes requires knowing data type at planning time, which is not applicable for schemaless data sources. The switch to use index as unique identifier is unavoidable in this case because column names are not guaranteed to be unique anymore once relation is introduced. For example, two joined tables could both introduce columns with the same names. I will do more research and take a look at Drill's source code to see if there are better ways to handle schemaless data sources.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-828880763
@jorgecarleitao looking more into the logical optimization invariants, I think we might want to relax it a little bit to account for some optimizations may change column orders: https://github.com/apache/arrow-datafusion/blob/57eeb64659b9ca9c496a959f7716090fb32085b6/datafusion/src/optimizer/hash_build_probe_order.rs#L122-L133.
So basically something like this:
* If projection is the outer plan, then we can guarantee strict schema invariants for logical optimization, i.e. we preserve the exact same schema field vector.
* If outer plan is not projection, we only guarantee same set of schema fields to be preserved, but not the order
From the user's point of view, it also makes sense since if I am executing a query like `SELECT * FROM t`, I am basically saying just give me all the columns in whatever order.
Technically, we could still enforce strict schema invariants for all plans by manually wrapping a projection plan when the outer plan is not a projection. But I think this adds unnecessary execution overhead for minor semantic gain.
Interesting in what others think about this.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] codecov-commenter commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-855311829
# [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/55?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#55](https://codecov.io/gh/apache/arrow-datafusion/pull/55?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9513c19) into [master](https://codecov.io/gh/apache/arrow-datafusion/commit/a9d04ca570015be6c94b767fef079783efdd877c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a9d04ca) will **increase** coverage by `0.10%`.
> The diff coverage is `77.20%`.
[](https://codecov.io/gh/apache/arrow-datafusion/pull/55?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #55 +/- ##
==========================================
+ Coverage 76.07% 76.18% +0.10%
==========================================
Files 155 155
Lines 26544 27237 +693
==========================================
+ Hits 20194 20750 +556
- Misses 6350 6487 +137
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-datafusion/pull/55?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [datafusion/src/optimizer/eliminate\_limit.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/55/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvb3B0aW1pemVyL2VsaW1pbmF0ZV9saW1pdC5ycw==) | `89.18% <ø> (ø)` | |
| [datafusion/src/optimizer/limit\_push\_down.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/55/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvb3B0aW1pemVyL2xpbWl0X3B1c2hfZG93bi5ycw==) | `97.84% <ø> (ø)` | |
| [datafusion/src/optimizer/simplify\_expressions.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/55/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvb3B0aW1pemVyL3NpbXBsaWZ5X2V4cHJlc3Npb25zLnJz) | `87.45% <ø> (ø)` | |
| [...afusion/src/physical\_optimizer/coalesce\_batches.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/55/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfb3B0aW1pemVyL2NvYWxlc2NlX2JhdGNoZXMucnM=) | `88.23% <ø> (ø)` | |
| [...atafusion/src/physical\_plan/expressions/min\_max.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/55/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9leHByZXNzaW9ucy9taW5fbWF4LnJz) | `95.39% <ø> (ø)` | |
| [datafusion/src/physical\_plan/expressions/mod.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/55/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9leHByZXNzaW9ucy9tb2QucnM=) | `71.42% <ø> (ø)` | |
| [datafusion/src/physical\_plan/mod.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/55/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9zcmMvcGh5c2ljYWxfcGxhbi9tb2QucnM=) | `78.70% <ø> (ø)` | |
| [ballista/rust/core/src/serde/mod.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/55/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmFsbGlzdGEvcnVzdC9jb3JlL3NyYy9zZXJkZS9tb2QucnM=) | `23.92% <23.71%> (-4.65%)` | :arrow_down: |
| [...ta/rust/core/src/serde/physical\_plan/from\_proto.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/55/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmFsbGlzdGEvcnVzdC9jb3JlL3NyYy9zZXJkZS9waHlzaWNhbF9wbGFuL2Zyb21fcHJvdG8ucnM=) | `31.59% <27.17%> (-7.06%)` | :arrow_down: |
| [...ista/rust/core/src/serde/physical\_plan/to\_proto.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/55/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmFsbGlzdGEvcnVzdC9jb3JlL3NyYy9zZXJkZS9waHlzaWNhbF9wbGFuL3RvX3Byb3RvLnJz) | `50.14% <42.30%> (-0.17%)` | :arrow_down: |
| ... and [53 more](https://codecov.io/gh/apache/arrow-datafusion/pull/55/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/55?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/55?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [a9d04ca...9513c19](https://codecov.io/gh/apache/arrow-datafusion/pull/55?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on a change in pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#discussion_r619864093
##########
File path: datafusion/src/execution/dataframe_impl.rs
##########
@@ -252,7 +252,12 @@ mod tests {
let right = test_table()?.select_columns(&["c1", "c3"])?;
let left_rows = left.collect().await?;
let right_rows = right.collect().await?;
- let join = left.join(right, JoinType::Inner, &["c1"], &["c1"])?;
+ let join = left.join(
+ right,
+ JoinType::Inner,
+ vec![Column::from_name("c1".to_string())],
Review comment:
That's a great idea!
##########
File path: datafusion/src/physical_optimizer/coalesce_batches.rs
##########
@@ -58,7 +59,17 @@ impl PhysicalOptimizerRule for CoalesceBatches {
// See https://issues.apache.org/jira/browse/ARROW-11068
let wrap_in_coalesce = plan_any.downcast_ref::<FilterExec>().is_some()
|| plan_any.downcast_ref::<HashJoinExec>().is_some()
- || plan_any.downcast_ref::<RepartitionExec>().is_some();
+ || {
+ match plan_any.downcast_ref::<RepartitionExec>() {
+ Some(p) => match p.partitioning() {
+ // do not coalesce hash partitions since other plans like partitioned hash
Review comment:
Good call, I started with handling this in `HashJoinStream`, but couldn't get it to work in all cases due to a small bug. I took a second look at my first attempt and noticed it could be fixed by changing 3 characters :P I have now reverted my change in the coalesce optimizer and added back my initial fix through https://github.com/apache/arrow-datafusion/pull/55/commits/696f8e04178fef877a658d0ef08c3c4ceebfcfdf.
Let me know if this fix aligns with what you have in mind. Also I suck at naming, `preserve_left` is the best I can come up with, suggestions welcome :)
##########
File path: datafusion/src/execution/dataframe_impl.rs
##########
@@ -252,7 +252,12 @@ mod tests {
let right = test_table()?.select_columns(&["c1", "c3"])?;
let left_rows = left.collect().await?;
let right_rows = right.collect().await?;
- let join = left.join(right, JoinType::Inner, &["c1"], &["c1"])?;
+ let join = left.join(
+ right,
+ JoinType::Inner,
+ vec![Column::from_name("c1".to_string())],
Review comment:
Turns out it is not possible to use `Vec<impl Into<Column>>` in the signature because Dataframe is used as trait object. I changed the signature back to `&[&str]` for dataframe API and switched to `Vec<impl Into<Column>>` in the logical plan builder to keep the high level API simple.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on a change in pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#discussion_r619791903
##########
File path: datafusion/src/physical_optimizer/coalesce_batches.rs
##########
@@ -58,7 +59,17 @@ impl PhysicalOptimizerRule for CoalesceBatches {
// See https://issues.apache.org/jira/browse/ARROW-11068
let wrap_in_coalesce = plan_any.downcast_ref::<FilterExec>().is_some()
|| plan_any.downcast_ref::<HashJoinExec>().is_some()
- || plan_any.downcast_ref::<RepartitionExec>().is_some();
+ || {
+ match plan_any.downcast_ref::<RepartitionExec>() {
+ Some(p) => match p.partitioning() {
+ // do not coalesce hash partitions since other plans like partitioned hash
Review comment:
I think this should be something to be solved in the hash join implementation, so if we merge this as is I think that deserves a github issue.
Also you might want to use the matches macro here: `if matches!(p.partitioning(), Partitioning::Hash(_, _))`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-827170656
FWIW I plan to look at this tomorrow more carefully
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on a change in pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#discussion_r621470506
##########
File path: datafusion/src/logical_plan/plan.rs
##########
@@ -141,7 +137,7 @@ pub enum LogicalPlan {
/// Produces rows from a table provider by reference or from the context
TableScan {
/// The name of the table
- table_name: String,
+ table_name: Option<String>,
Review comment:
This is needed because all the non-table scan all eventually goes into TableScan, for example: https://github.com/apache/arrow-datafusion/blob/245f0b8a68c5763a236aef3e727f0502188d0bfa/datafusion/src/logical_plan/builder.rs#L110-L117. Currently we are using empty str `''` to imply scan without table names, which makes the relation name match a little bit less readable downstream in the code base.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp edited a comment on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp edited a comment on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-860150632
@andygrove @Dandandan @jorgecarleitao @alamb @Jimexist this PR is now ready for review. We are now able to pass both tpch-7 and tpch-8.
I filed https://github.com/apache/arrow-datafusion/issues/549 to track compound column support as a fast follow task since this PR is already very large and keeps growing every time I rebase on top of the latest master. If you all think I should include compound column support in this PR, I am more than happy to add it.
Output field name are not 100% conforming to https://github.com/apache/arrow-datafusion/blob/master/docs/specification/output-field-name-semantic.md yet. The remaining differences are all minor formatting issues like converting function names to lower cases, so I decided to leave that to follow up PRs to reduce the diff.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-860846672
I suggest we get this PR rebased and merged asap to minimize conflicts
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp edited a comment on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp edited a comment on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-828880763
@jorgecarleitao looking more into the logical optimization invariants, I think we might want to relax it a little bit to account for some optimizations that may change column orders: https://github.com/apache/arrow-datafusion/blob/57eeb64659b9ca9c496a959f7716090fb32085b6/datafusion/src/optimizer/hash_build_probe_order.rs#L122-L133.
So basically something like this:
* If projection is the outer plan, then we can guarantee strict schema invariants for logical optimization, i.e. we preserve the exact same schema field vector.
* If outer plan is not projection, we only guarantee same set of schema fields to be preserved, but not the order
From the user's point of view, it also makes sense since if I am executing a query like `SELECT * FROM t`, I am basically saying just give me all the columns in whatever order.
Technically, we could still enforce strict schema invariants for all plans by manually wrapping a projection plan when the outer plan is not a projection. But I think this adds unnecessary execution overhead for minor semantic gain.
Interesting in what others think about this.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-829334791
@andygrove could you give some examples for schemaless data sources? I would like to incorporate that into the design doc.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-864515901
looks like integration test is failing due to mismatch between logical schema and physical schema, i will dig into this.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on a change in pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#discussion_r650435911
##########
File path: ballista/rust/core/src/serde/logical_plan/from_proto.rs
##########
@@ -1361,43 +1224,3 @@ impl TryFrom<protobuf::WindowFrame> for WindowFrame {
})
}
}
-
-impl From<protobuf::AggregateFunction> for AggregateFunction {
- fn from(aggr_function: protobuf::AggregateFunction) -> Self {
- match aggr_function {
- protobuf::AggregateFunction::Min => AggregateFunction::Min,
- protobuf::AggregateFunction::Max => AggregateFunction::Max,
- protobuf::AggregateFunction::Sum => AggregateFunction::Sum,
- protobuf::AggregateFunction::Avg => AggregateFunction::Avg,
- protobuf::AggregateFunction::Count => AggregateFunction::Count,
- }
- }
-}
-
-impl From<protobuf::BuiltInWindowFunction> for BuiltInWindowFunction {
- fn from(built_in_function: protobuf::BuiltInWindowFunction) -> Self {
- match built_in_function {
- protobuf::BuiltInWindowFunction::RowNumber => {
- BuiltInWindowFunction::RowNumber
- }
- protobuf::BuiltInWindowFunction::Rank => BuiltInWindowFunction::Rank,
- protobuf::BuiltInWindowFunction::PercentRank => {
- BuiltInWindowFunction::PercentRank
- }
- protobuf::BuiltInWindowFunction::DenseRank => {
- BuiltInWindowFunction::DenseRank
- }
- protobuf::BuiltInWindowFunction::Lag => BuiltInWindowFunction::Lag,
- protobuf::BuiltInWindowFunction::Lead => BuiltInWindowFunction::Lead,
- protobuf::BuiltInWindowFunction::FirstValue => {
- BuiltInWindowFunction::FirstValue
- }
- protobuf::BuiltInWindowFunction::CumeDist => BuiltInWindowFunction::CumeDist,
- protobuf::BuiltInWindowFunction::Ntile => BuiltInWindowFunction::Ntile,
- protobuf::BuiltInWindowFunction::NthValue => BuiltInWindowFunction::NthValue,
- protobuf::BuiltInWindowFunction::LastValue => {
- BuiltInWindowFunction::LastValue
- }
- }
- }
-}
Review comment:
all these shared code are moved into `ballista/rust/core/src/serde/mod.rs`.
##########
File path: datafusion/src/logical_plan/builder.rs
##########
@@ -210,69 +238,103 @@ impl LogicalPlanBuilder {
}
/// Apply a sort
- pub fn sort(&self, expr: impl IntoIterator<Item = Expr>) -> Result<Self> {
+ pub fn sort(&self, exprs: impl IntoIterator<Item = Expr>) -> Result<Self> {
+ let schemas = self.plan.all_schemas();
Ok(Self::from(&LogicalPlan::Sort {
- expr: expr.into_iter().collect(),
+ expr: normalize_cols(exprs, &schemas)?,
input: Arc::new(self.plan.clone()),
}))
}
/// Apply a union
pub fn union(&self, plan: LogicalPlan) -> Result<Self> {
- let schema = self.plan.schema();
Review comment:
refactored implementation into `union_with_alias` function so the same logic can be reused in sql planner.
##########
File path: datafusion/src/physical_plan/planner.rs
##########
@@ -56,6 +56,121 @@ use expressions::col;
use log::debug;
use std::sync::Arc;
+fn create_function_physical_name(
+ fun: &str,
+ distinct: bool,
+ args: &[Expr],
+ input_schema: &DFSchema,
+) -> Result<String> {
+ let names: Vec<String> = args
+ .iter()
+ .map(|e| physical_name(e, input_schema))
+ .collect::<Result<_>>()?;
+
+ let distinct_str = match distinct {
+ true => "DISTINCT ",
+ false => "",
+ };
+ Ok(format!("{}({}{})", fun, distinct_str, names.join(",")))
+}
+
+fn physical_name(e: &Expr, input_schema: &DFSchema) -> Result<String> {
Review comment:
This function is responsible for generating physical field name for a given expression based on https://github.com/apache/arrow-datafusion/blob/master/docs/specification/output-field-name-semantic.md.
I kept `Expr.name` as is for better readability in query explain and debug output.
##########
File path: datafusion/src/physical_plan/planner.rs
##########
@@ -501,17 +684,20 @@ impl DefaultPhysicalPlanner {
pub fn create_physical_expr(
&self,
e: &Expr,
+ input_dfschema: &DFSchema,
Review comment:
`create_physical_expr` now requires logical plan schema for column index lookup
##########
File path: datafusion/src/sql/planner.rs
##########
@@ -424,17 +408,24 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
ctes: &mut HashMap<String, LogicalPlan>,
) -> Result<LogicalPlan> {
match relation {
- TableFactor::Table { name, .. } => {
+ TableFactor::Table { name, alias, .. } => {
let table_name = name.to_string();
let cte = ctes.get(&table_name);
match (
cte,
self.schema_provider.get_table_provider(name.try_into()?),
) {
(Some(cte_plan), _) => Ok(cte_plan.clone()),
- (_, Some(provider)) => {
- LogicalPlanBuilder::scan(&table_name, provider, None)?.build()
- }
+ (_, Some(provider)) => LogicalPlanBuilder::scan(
+ // take alias into account to support `JOIN table1 as table2`
+ alias
+ .as_ref()
+ .map(|a| a.name.value.as_str())
+ .unwrap_or(&table_name),
+ provider,
+ None,
+ )?
+ .build(),
Review comment:
this change has been split out into https://github.com/apache/arrow-datafusion/pull/547 for easier review.
##########
File path: datafusion/src/logical_plan/expr.rs
##########
@@ -1240,7 +1394,7 @@ impl fmt::Debug for Expr {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match self {
Expr::Alias(expr, alias) => write!(f, "{:?} AS {}", expr, alias),
- Expr::Column(name) => write!(f, "#{}", name),
+ Expr::Column(c) => write!(f, "{}", c),
Review comment:
`#` prefix is now provided by Column's `fmt` method.
##########
File path: datafusion/src/physical_plan/expressions/column.rs
##########
@@ -61,26 +68,21 @@ impl PhysicalExpr for Column {
/// Get the data type of this expression, given the schema of the input
fn data_type(&self, input_schema: &Schema) -> Result<DataType> {
- Ok(input_schema
- .field_with_name(&self.name)?
- .data_type()
- .clone())
+ Ok(input_schema.field(self.index).data_type().clone())
}
/// Decide whehter this expression is nullable, given the schema of the input
fn nullable(&self, input_schema: &Schema) -> Result<bool> {
- Ok(input_schema.field_with_name(&self.name)?.is_nullable())
+ Ok(input_schema.field(self.index).is_nullable())
}
/// Evaluate the expression
fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
- Ok(ColumnarValue::Array(
- batch.column(batch.schema().index_of(&self.name)?).clone(),
- ))
+ Ok(ColumnarValue::Array(batch.column(self.index).clone()))
Review comment:
Physical column expression evaluation now indexes record batch column directly instead of doing string comparisons on all field names.
##########
File path: datafusion/src/physical_plan/functions.rs
##########
@@ -512,39 +512,35 @@ macro_rules! invoke_if_unicode_expressions_feature_flag {
};
}
-/// Create a physical (function) expression.
-/// This function errors when `args`' can't be coerced to a valid argument type of the function.
-pub fn create_physical_expr(
+/// Create a physical scalar function.
Review comment:
this change has been split out into https://github.com/apache/arrow-datafusion/pull/548 for easier review.
##########
File path: ballista/rust/core/src/serde/logical_plan/from_proto.rs
##########
@@ -1361,43 +1224,3 @@ impl TryFrom<protobuf::WindowFrame> for WindowFrame {
})
}
}
-
-impl From<protobuf::AggregateFunction> for AggregateFunction {
- fn from(aggr_function: protobuf::AggregateFunction) -> Self {
- match aggr_function {
- protobuf::AggregateFunction::Min => AggregateFunction::Min,
- protobuf::AggregateFunction::Max => AggregateFunction::Max,
- protobuf::AggregateFunction::Sum => AggregateFunction::Sum,
- protobuf::AggregateFunction::Avg => AggregateFunction::Avg,
- protobuf::AggregateFunction::Count => AggregateFunction::Count,
- }
- }
-}
-
-impl From<protobuf::BuiltInWindowFunction> for BuiltInWindowFunction {
- fn from(built_in_function: protobuf::BuiltInWindowFunction) -> Self {
- match built_in_function {
- protobuf::BuiltInWindowFunction::RowNumber => {
- BuiltInWindowFunction::RowNumber
- }
- protobuf::BuiltInWindowFunction::Rank => BuiltInWindowFunction::Rank,
- protobuf::BuiltInWindowFunction::PercentRank => {
- BuiltInWindowFunction::PercentRank
- }
- protobuf::BuiltInWindowFunction::DenseRank => {
- BuiltInWindowFunction::DenseRank
- }
- protobuf::BuiltInWindowFunction::Lag => BuiltInWindowFunction::Lag,
- protobuf::BuiltInWindowFunction::Lead => BuiltInWindowFunction::Lead,
- protobuf::BuiltInWindowFunction::FirstValue => {
- BuiltInWindowFunction::FirstValue
- }
- protobuf::BuiltInWindowFunction::CumeDist => BuiltInWindowFunction::CumeDist,
- protobuf::BuiltInWindowFunction::Ntile => BuiltInWindowFunction::Ntile,
- protobuf::BuiltInWindowFunction::NthValue => BuiltInWindowFunction::NthValue,
- protobuf::BuiltInWindowFunction::LastValue => {
- BuiltInWindowFunction::LastValue
- }
- }
- }
-}
Review comment:
all these shared code are moved into `ballista/rust/core/src/serde/mod.rs` so it can be shared with physical plan ser/de.
##########
File path: ballista/rust/core/proto/ballista.proto
##########
@@ -408,6 +421,119 @@ message PhysicalPlanNode {
}
}
+// physical expressions
Review comment:
Physical expressions are now serialized and deserialized without involving logical expressions. It is needed because logical columns and physical columns are represented differently, one with relation qualifier and the other one with column index. This should also speed up the deserialization for physical plane because we won't need to run physical expressions through physical planner anymore.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on a change in pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#discussion_r619791903
##########
File path: datafusion/src/physical_optimizer/coalesce_batches.rs
##########
@@ -58,7 +59,17 @@ impl PhysicalOptimizerRule for CoalesceBatches {
// See https://issues.apache.org/jira/browse/ARROW-11068
let wrap_in_coalesce = plan_any.downcast_ref::<FilterExec>().is_some()
|| plan_any.downcast_ref::<HashJoinExec>().is_some()
- || plan_any.downcast_ref::<RepartitionExec>().is_some();
+ || {
+ match plan_any.downcast_ref::<RepartitionExec>() {
+ Some(p) => match p.partitioning() {
+ // do not coalesce hash partitions since other plans like partitioned hash
Review comment:
I think this should be something to be solved in the hash join implementation, so if we merge this as is I think that deserves a github issue.
Also you might want to use the matches macro here: `!matches!(p.partitioning(), Partitioning::Hash(_, _))`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp edited a comment on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp edited a comment on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-831405269
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on a change in pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#discussion_r619791415
##########
File path: datafusion/src/physical_optimizer/coalesce_batches.rs
##########
@@ -58,7 +59,17 @@ impl PhysicalOptimizerRule for CoalesceBatches {
// See https://issues.apache.org/jira/browse/ARROW-11068
let wrap_in_coalesce = plan_any.downcast_ref::<FilterExec>().is_some()
|| plan_any.downcast_ref::<HashJoinExec>().is_some()
- || plan_any.downcast_ref::<RepartitionExec>().is_some();
+ || {
+ match plan_any.downcast_ref::<RepartitionExec>() {
+ Some(p) => match p.partitioning() {
+ // do not coalesce hash partitions since other plans like partitioned hash
+ // join depends on it empty batches for outter joins
Review comment:
```suggestion
// join depends on it empty batches for outer joins
```
##########
File path: datafusion/src/physical_optimizer/coalesce_batches.rs
##########
@@ -58,7 +59,17 @@ impl PhysicalOptimizerRule for CoalesceBatches {
// See https://issues.apache.org/jira/browse/ARROW-11068
let wrap_in_coalesce = plan_any.downcast_ref::<FilterExec>().is_some()
|| plan_any.downcast_ref::<HashJoinExec>().is_some()
- || plan_any.downcast_ref::<RepartitionExec>().is_some();
+ || {
+ match plan_any.downcast_ref::<RepartitionExec>() {
+ Some(p) => match p.partitioning() {
+ // do not coalesce hash partitions since other plans like partitioned hash
Review comment:
I think this should be something to be solved in the hash join implementation, so if we merge this as is I think that deserves a github issue.
Also you might want to use the matches macro here: `if matches!(p.partitioning(), Partitioning::Hash(_, _))`
##########
File path: datafusion/src/physical_optimizer/coalesce_batches.rs
##########
@@ -58,7 +59,17 @@ impl PhysicalOptimizerRule for CoalesceBatches {
// See https://issues.apache.org/jira/browse/ARROW-11068
let wrap_in_coalesce = plan_any.downcast_ref::<FilterExec>().is_some()
|| plan_any.downcast_ref::<HashJoinExec>().is_some()
- || plan_any.downcast_ref::<RepartitionExec>().is_some();
+ || {
+ match plan_any.downcast_ref::<RepartitionExec>() {
+ Some(p) => match p.partitioning() {
+ // do not coalesce hash partitions since other plans like partitioned hash
Review comment:
I think this should be something to be solved in the hash join implementation, so if we merge this as is I think that deserves a github issue.
Also you might want to use the matches macro here: `!matches!(p.partitioning(), Partitioning::Hash(_, _))`
##########
File path: datafusion/src/execution/dataframe_impl.rs
##########
@@ -252,7 +252,12 @@ mod tests {
let right = test_table()?.select_columns(&["c1", "c3"])?;
let left_rows = left.collect().await?;
let right_rows = right.collect().await?;
- let join = left.join(right, JoinType::Inner, &["c1"], &["c1"])?;
+ let join = left.join(
+ right,
+ JoinType::Inner,
+ vec![Column::from_name("c1".to_string())],
Review comment:
I think it might be useful to keep supporting the string syntax to keep the dataframe api simple, but also support qualified references.
Maybe the API could be used `column: Vec<Into<Column>>` and includes an implementation for `&str`?
##########
File path: datafusion/src/execution/dataframe_impl.rs
##########
@@ -252,7 +252,12 @@ mod tests {
let right = test_table()?.select_columns(&["c1", "c3"])?;
let left_rows = left.collect().await?;
let right_rows = right.collect().await?;
- let join = left.join(right, JoinType::Inner, &["c1"], &["c1"])?;
+ let join = left.join(
+ right,
+ JoinType::Inner,
+ vec![Column::from_name("c1".to_string())],
Review comment:
I think it might be useful to keep supporting the string syntax to keep the dataframe api simple, but also support qualified names.
Maybe the API could be used `column: Vec<Into<Column>>` and includes an implementation for `&str`?
##########
File path: datafusion/src/physical_plan/hash_join.rs
##########
@@ -891,6 +898,36 @@ impl Stream for HashJoinStream {
}
Some(result)
}
+ // If maybe_batch is None and num_output_rows is 0, that means right side batch was
+ // empty and has been coalesced to None. Fill right side with Null if preserve_left
+ // is true.
+ None if self.preserve_left && self.num_output_rows == 0 => {
Review comment:
I think this partially resolves a more general issue with the left join, which is that it doesn't keep track of unmatched left rows across batches. https://issues.apache.org/jira/browse/ARROW-10971
Maybe we can add a TODO here / issue that we should generalize this to produce rows that were not matched.
This looks like a great start for that 👍
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-836102431
Looks like everyone is happy with field name style doc (https://docs.google.com/document/d/1uviWavwEGD3qxwMk2AGkOgp6ENrvKGiMWQhHNbqPwhg) so far.
Going to wait 1-2 days for more comments on invariant update doc (https://docs.google.com/document/d/1dbK-3eaTHlzZcHzpTk1h-LA3b7dcxsVBcoZeVKYIPwI) before I do a final round of editing then implement all the proposed changes.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp edited a comment on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp edited a comment on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-827766515
Thanks @jorgecarleitao for the invariant design doc, I wasn't aware of it before. I will definitely go through it and write something up for this PR this weekend. I think we should spend some effort into adding that doc into this repo as well. This is the kind of reference I have wanted to have while working on the patch set.
My first attempt of this does preserve the invariant of `SELECT t.a FROM t must result in the schema field name t.a`, but I later found out this would cause issue with subsequent read from writes. Basically the reader will end up with something like `t.t.a`. So I changed the behavior to be consistent with Spark and MySQL.
For breaking changes in schema fields involving operators, I will do more research on my end. The current behavior is the one with the least amount of work for me. But I am leaning towards better compatibility with the current data eng ecosystem like spark, mysql and postgres.
I fully agree with the optimization invariants. I have been thinking about whether there is a better way to enforce them more formally other than unit tests.
@jorgecarleitao to your last question, all the invariants, other than the first one I mentioned in this comment, are pretty straight forward to maintain by tracking the user provided input in the column struct. I didn't do it because I wasn't aware of these invariants.
@alamb I am with you for the size of this PR as well :( That's why I decided to send it out early before finishing it because I noticed it just kept growing in size especially after every time I merge with master :P. I will try to see if I can restrict the change to just logical plans, but as @jorgecarleitao mentioned it might not be easy. I will do it if the overhead turned out to be small.
Anyway, putting code change aside, I think the most urgent thing to do is for me to write up a design doc to fully spec out the semantics so we are all on the same page on what needs to be implemented.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on pull request #55: Support qualified columns in queries
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #55:
URL: https://github.com/apache/arrow-datafusion/pull/55#issuecomment-861203825
@alamb good catch on the join using bug, I will get it fixed in this PR.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org