Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

consolidate binary_expr coercion rule code into binary_rule.rs module #1607

Merged
merged 4 commits into from
Jan 20, 2022

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Jan 18, 2022

Builds on #1606, so draft until that is merged

Which issue does this PR close?

Re #1605

Rationale for this change

While working on #1606 I found it confusing that some of the logic was in coercion.rs and some in binary_rule.rs

What changes are included in this PR?

Moves all binary operator coercion logic into binary_rule.rs (just code motion, no changes now). I hope that the logic is consolidated more in a future PR

Are there any user-facing changes?

no

@alamb alamb marked this pull request as draft January 18, 2022 15:30
@alamb
Copy link
Contributor Author

alamb commented Jan 18, 2022

cc @liukun4515

@alamb alamb changed the title Move code to consolidate binary_expr coercion rules consolidate binary_expr coercion rules Jan 18, 2022
@alamb alamb changed the title consolidate binary_expr coercion rules consolidate binary_expr coercion rule code Jan 18, 2022
@github-actions github-actions bot added the datafusion Changes in the datafusion crate label Jan 18, 2022
@liukun4515
Copy link
Contributor

Thanks @alamb
I think it's my fault and leave some duplicated code.
My original thought is that I will consolidate binary logic and coercion in the follow-up pull-requests.

@liukun4515
Copy link
Contributor

If #1606 merged, I will review this.

@alamb
Copy link
Contributor Author

alamb commented Jan 19, 2022

I think it's my fault and leave some duplicated code.
My original thought is that I will consolidate binary logic and coercion in the follow-up pull-requests.

No worries @liukun4515 -- you said that. I just figured I was already changing the code for #1606 so I would make progress here too

Also note there is no actual consolidation of the logic -- I just moved the code to the same module. I envision your cleanup to consolidate the actual logic would be in a follow on PR

@alamb alamb force-pushed the alamb/condolidate_coercion branch from 0c3ee94 to 90d9a27 Compare January 19, 2022 12:18
@alamb alamb changed the title consolidate binary_expr coercion rule code consolidate binary_expr coercion rule code into binary_rule.rs module Jan 19, 2022
@alamb alamb force-pushed the alamb/condolidate_coercion branch from 90d9a27 to c065f05 Compare January 19, 2022 13:40
@alamb alamb marked this pull request as ready for review January 19, 2022 13:40
Copy link
Member

@xudong963 xudong963 left a comment

Choose a reason for hiding this comment

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

LGTM, so much cleaner

datafusion/src/physical_plan/coercion_rule/binary_rule.rs Outdated Show resolved Hide resolved
@@ -293,14 +289,192 @@ fn coercion_decimal_mathematics_type(
}
}

/// Determine if a DataType is signed numeric or not
pub fn is_signed_numeric(dt: &DataType) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

}

/// Determine if a DataType is numeric or not
pub fn is_numeric(dt: &DataType) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

}
}

pub(crate) fn is_dictionary(t: &DataType) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@liukun4515 liukun4515 left a comment

Choose a reason for hiding this comment

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

LGTM

@alamb alamb merged commit eb51fae into apache:master Jan 20, 2022
@alamb alamb deleted the alamb/condolidate_coercion branch January 20, 2022 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datafusion Changes in the datafusion crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants