Skip to content

Conversation

@jorgecarleitao
Copy link
Member

@jorgecarleitao jorgecarleitao commented Aug 22, 2020

This commit makes all type coercion happen on the physical plane instead of logical plane and fixes the supertype function. This makes field names to not change due to coercion rules, better control of how the coercion supports physical calculations, and others.

This commit also makes it more clear how we enforce type checking during planning. the Logical plan now knows how to derive its schema directly from binary expressions, even before the coercion is applied.

The rational for this change is that coercions are simplifications to a physical computation (it is easier to sum two numbers of the same type at the hardware level).

This partially solves ARROW-9809 (for binary expressions, not for udfs), an issue on which the physical schema could be modified by coercion rules, causing the RecordBatch's schema to be different from the logical batch.

This also addresses some inconsistencies in how we coerced certain types for binary operators, causing such inconsistencies to error during planning instead of execution.

This also introduces a significant number of tests into the overall consistency of binary operators: it is now explicit what types they expect and how coercion happens to each operator. It also adds tests to different parts of the physical execution, to ensure schema consistency for binary operators, including negative tests (when it should error).

This also makes like and nlike generally available, and added some tests to it.

This closes ARROW-4957.

@andygrove and @alamb, I am really sorry for this long commit, but I was unable to split this in smaller parts with passing tests. There was a strong coupling between the get_supertype and the physical expressions that made it hard to work this through.

@github-actions
Copy link

Copy link
Member

@andygrove andygrove left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a fantastic improvement! Thanks @jorgecarleitao

This commit makes all type coercion happen on the
physical plane instead of logical plane. This allows field
names to not change due to coercion rules.

The rational for this change is that coercions are simplifications
to a physical computation (it is easier to sum two numbers
of the same type at the hardware level).

This commit essentially makes the logical plane to not worry
about type coercion, only about the resulting type of the operator.

This also addresses an issue on which the physical schema could
be modified by coercion rules, causing the RecordBatch's schema
to be different from the logical batch.

This also addresses some inconsistencies in how we coerced certain
types for binary operators, causing such inconsistencies to error
during planning instead of during execution.

This closes ARROW-9809 and ARROW-4957.
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't say I totally follow all this code and I didn't study the diff all that carefully, but I also don't have any opposition to merging this. The PR also increases test coverage, so I say :shipit:

In general, architecturally it sounds a little strange to me to postpone type coercion (also known as type resolution) to physical planning, as I think the information is useful during logical planning (e.g it is important to know if we want to do partial evaluation such as turning A < 5 OR A = 5 into A <= 5 I think you need to know the actual types of A and 5.

However, since most of the logic operates on DataType which is shared between Logical and Physical plans, I think we can always move where exactly the code is executed (any maybe even run it in both places).

@jorgecarleitao
Copy link
Member Author

@alamb , thanks a lot for that insight.

I may have been using the wrong notation here.

I think that we have each columns' type during logical planning: the LogicalPlanBuilder always starts with a scan with a well defined (or infered via scan) schema. When a projection is constructed, which requires us to derive a schema, we build that schema by deriving the column types from its expressions, via exprlist_to_fields (that uses Expr::to_field that uses Expr::get_type(input_schema)).

As I see it, the type coercer optimizer is casting types being passed to binary operators for the sole purpose of matching numerical types to perform computations, as we do not have kernels for different numerical types (e.g. u16 + u32).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants