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 2022/11/08 15:54:28 UTC
[GitHub] [arrow-datafusion] alamb opened a new pull request, #4145: Update docstring on unwrap_cast_in_comparison
alamb opened a new pull request, #4145:
URL: https://github.com/apache/arrow-datafusion/pull/4145
# Which issue does this PR close?
Related to https://github.com/apache/arrow-datafusion/issues/3938
# Rationale for this change
I am working in this code and figured I would encode some of my learning
# What changes are included in this PR?
Improve the docstrings
# Are these changes tested?
No code change
# Are there any user-facing changes?
No
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #4145: Minor: Update docstring on unwrap_cast_in_comparison
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #4145:
URL: https://github.com/apache/arrow-datafusion/pull/4145#discussion_r1017074282
##########
datafusion/optimizer/src/unwrap_cast_in_comparison.rs:
##########
@@ -32,25 +32,40 @@ use datafusion_expr::{
};
use std::sync::Arc;
-/// The rule can be used to the numeric binary comparison with literal expr, like below pattern:
-/// `cast(left_expr as data_type) comparison_op literal_expr` or `literal_expr comparison_op cast(right_expr as data_type)`.
-/// The data type of two sides must be equal, and must be signed numeric type now, and will support more data type later.
+/// [`UnwrapCastInComparison`] Attempts to remove casts from
+/// comparisons to literals ([`ScalarValue`]s) by applying the casts
+/// to the literals if possible. It is inspired by the optimizer rule
+/// `UnwrapCastInBinaryComparison` of Spark.
///
-/// If the binary comparison expr match above rules, the optimizer will check if the value of `literal`
-/// is in within range(min,max) which is the range(min,max) of the data type for `left_expr` or `right_expr`.
+/// Removing casts often improves performance because:
+/// 1. The cast is done once (to the literal) rather than to every value
+/// 2. Can enable other optimizations such as predicate pushdown that
+/// don't support casting
///
-/// If this is true, the literal expr will be casted to the data type of expr on the other side, and the result of
-/// binary comparison will be `left_expr comparison_op cast(literal_expr, left_data_type)` or
-/// `cast(literal_expr, right_data_type) comparison_op right_expr`. For better optimization,
-/// the expr of `cast(literal_expr, target_type)` will be precomputed and converted to the new expr `new_literal_expr`
-/// which data type is `target_type`.
-/// If this false, do nothing.
+/// The rule is applied to expressions of the following forms:
+///
+/// 1. `cast(left_expr as data_type) comparison_op literal_expr`
+/// 2. `cast(literal_expr) IN (expr1, expr2, ...)`
+///
+/// If the expression matches one of the forms above, the rule will
+/// ensure the value of `literal` is in within range(min,max) of the
Review Comment:
```suggestion
/// ensure the value of `literal` is in within range(min, max) of the
```
##########
datafusion/optimizer/src/unwrap_cast_in_comparison.rs:
##########
@@ -32,25 +32,40 @@ use datafusion_expr::{
};
use std::sync::Arc;
-/// The rule can be used to the numeric binary comparison with literal expr, like below pattern:
-/// `cast(left_expr as data_type) comparison_op literal_expr` or `literal_expr comparison_op cast(right_expr as data_type)`.
-/// The data type of two sides must be equal, and must be signed numeric type now, and will support more data type later.
+/// [`UnwrapCastInComparison`] Attempts to remove casts from
Review Comment:
```suggestion
/// [`UnwrapCastInComparison`] attempts to remove casts from
```
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on a diff in pull request #4145: Minor: Update docstring on unwrap_cast_in_comparison
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #4145:
URL: https://github.com/apache/arrow-datafusion/pull/4145#discussion_r1016921605
##########
datafusion/optimizer/src/unwrap_cast_in_comparison.rs:
##########
@@ -32,25 +32,40 @@ use datafusion_expr::{
};
use std::sync::Arc;
-/// The rule can be used to the numeric binary comparison with literal expr, like below pattern:
-/// `cast(left_expr as data_type) comparison_op literal_expr` or `literal_expr comparison_op cast(right_expr as data_type)`.
-/// The data type of two sides must be equal, and must be signed numeric type now, and will support more data type later.
+/// [`UnwrapCastInComparison`] Attempts to remove casts from
+/// comparisons to literals ([`ScalarValue`]s) by applying the casts
+/// to the literals if possible. It is inspired by the optimizer rule
+/// `UnwrapCastInBinaryComparison` of Spark.
///
-/// If the binary comparison expr match above rules, the optimizer will check if the value of `literal`
-/// is in within range(min,max) which is the range(min,max) of the data type for `left_expr` or `right_expr`.
+/// Removing casts often improves performance because:
+/// 1. The cast is done once (to the literal) rather than to every value
+/// 2. Can enable other optimizations such as predicate pushdown that
+/// don't support casting
///
-/// If this is true, the literal expr will be casted to the data type of expr on the other side, and the result of
-/// binary comparison will be `left_expr comparison_op cast(literal_expr, left_data_type)` or
-/// `cast(literal_expr, right_data_type) comparison_op right_expr`. For better optimization,
-/// the expr of `cast(literal_expr, target_type)` will be precomputed and converted to the new expr `new_literal_expr`
-/// which data type is `target_type`.
-/// If this false, do nothing.
+/// The rule is applied to expressions of the following forms:
+///
+/// 1. `cast(left_expr as data_type) comparison_op literal_expr`
+/// 2. `cast(literal_expr) IN (expr1, expr2, ...)`
Review Comment:
I guess that you mean:
```
2. `literal_expr IN (cast(expr1), cast(expr2), ...)`
```
?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #4145: Minor: Update docstring on unwrap_cast_in_comparison
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #4145:
URL: https://github.com/apache/arrow-datafusion/pull/4145#discussion_r1017077087
##########
datafusion/optimizer/src/unwrap_cast_in_comparison.rs:
##########
@@ -32,25 +32,40 @@ use datafusion_expr::{
};
use std::sync::Arc;
-/// The rule can be used to the numeric binary comparison with literal expr, like below pattern:
-/// `cast(left_expr as data_type) comparison_op literal_expr` or `literal_expr comparison_op cast(right_expr as data_type)`.
-/// The data type of two sides must be equal, and must be signed numeric type now, and will support more data type later.
+/// [`UnwrapCastInComparison`] Attempts to remove casts from
+/// comparisons to literals ([`ScalarValue`]s) by applying the casts
+/// to the literals if possible. It is inspired by the optimizer rule
+/// `UnwrapCastInBinaryComparison` of Spark.
///
-/// If the binary comparison expr match above rules, the optimizer will check if the value of `literal`
-/// is in within range(min,max) which is the range(min,max) of the data type for `left_expr` or `right_expr`.
+/// Removing casts often improves performance because:
+/// 1. The cast is done once (to the literal) rather than to every value
+/// 2. Can enable other optimizations such as predicate pushdown that
+/// don't support casting
///
-/// If this is true, the literal expr will be casted to the data type of expr on the other side, and the result of
-/// binary comparison will be `left_expr comparison_op cast(literal_expr, left_data_type)` or
-/// `cast(literal_expr, right_data_type) comparison_op right_expr`. For better optimization,
-/// the expr of `cast(literal_expr, target_type)` will be precomputed and converted to the new expr `new_literal_expr`
-/// which data type is `target_type`.
-/// If this false, do nothing.
+/// The rule is applied to expressions of the following forms:
+///
+/// 1. `cast(left_expr as data_type) comparison_op literal_expr`
+/// 2. `cast(literal_expr) IN (expr1, expr2, ...)`
Review Comment:
Yes, that is true.
```suggestion
/// 2. `literal_expr comparison_op cast(left_expr as data_type)`
/// 3. `cast(literal_expr) IN (expr1, expr2, ...)`
/// 4. `literal_expr IN (cast(expr1) , cast(expr2), ...)`
```
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #4145: Minor: Update docstring on unwrap_cast_in_comparison
Posted by GitBox <gi...@apache.org>.
alamb merged PR #4145:
URL: https://github.com/apache/arrow-datafusion/pull/4145
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] ursabot commented on pull request #4145: Minor: Update docstring on unwrap_cast_in_comparison
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #4145:
URL: https://github.com/apache/arrow-datafusion/pull/4145#issuecomment-1309101358
Benchmark runs are scheduled for baseline = 01941aae26ca14f765249b00c0ad83c28285167c and contender = 130354394ed8657a90e89a0bb65643e01715811a. 130354394ed8657a90e89a0bb65643e01715811a is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/28b13551e9cc477583d4884c20e5d68a...779fa06d42e84ca3a108e74c3fa06c96/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/ad3010c7afcd48e583ed1235ef415ece...5f645e789c1349118a3964848e43c0cd/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/32edb5a7d853462e975ebbbcba346141...3ccf4937a9134564a9b7857c28b0461f/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/ec1d10bcfdbc424cb63061c88a6d017b...03f2fc6b2c344f44a8bc98909150a2ed/)
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on a diff in pull request #4145: Minor: Update docstring on unwrap_cast_in_comparison
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #4145:
URL: https://github.com/apache/arrow-datafusion/pull/4145#discussion_r1016922259
##########
datafusion/optimizer/src/unwrap_cast_in_comparison.rs:
##########
@@ -32,25 +32,40 @@ use datafusion_expr::{
};
use std::sync::Arc;
-/// The rule can be used to the numeric binary comparison with literal expr, like below pattern:
-/// `cast(left_expr as data_type) comparison_op literal_expr` or `literal_expr comparison_op cast(right_expr as data_type)`.
-/// The data type of two sides must be equal, and must be signed numeric type now, and will support more data type later.
+/// [`UnwrapCastInComparison`] Attempts to remove casts from
+/// comparisons to literals ([`ScalarValue`]s) by applying the casts
+/// to the literals if possible. It is inspired by the optimizer rule
+/// `UnwrapCastInBinaryComparison` of Spark.
///
-/// If the binary comparison expr match above rules, the optimizer will check if the value of `literal`
-/// is in within range(min,max) which is the range(min,max) of the data type for `left_expr` or `right_expr`.
+/// Removing casts often improves performance because:
+/// 1. The cast is done once (to the literal) rather than to every value
+/// 2. Can enable other optimizations such as predicate pushdown that
+/// don't support casting
///
-/// If this is true, the literal expr will be casted to the data type of expr on the other side, and the result of
-/// binary comparison will be `left_expr comparison_op cast(literal_expr, left_data_type)` or
-/// `cast(literal_expr, right_data_type) comparison_op right_expr`. For better optimization,
-/// the expr of `cast(literal_expr, target_type)` will be precomputed and converted to the new expr `new_literal_expr`
-/// which data type is `target_type`.
-/// If this false, do nothing.
+/// The rule is applied to expressions of the following forms:
+///
+/// 1. `cast(left_expr as data_type) comparison_op literal_expr`
+/// 2. `cast(literal_expr) IN (expr1, expr2, ...)`
+///
+/// If the expression matches one of the forms above, the rule will
+/// ensure the value of `literal` is in within range(min,max) of the
Review Comment:
nit: (min,max) -> (min, max)
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on a diff in pull request #4145: Minor: Update docstring on unwrap_cast_in_comparison
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #4145:
URL: https://github.com/apache/arrow-datafusion/pull/4145#discussion_r1016921924
##########
datafusion/optimizer/src/unwrap_cast_in_comparison.rs:
##########
@@ -32,25 +32,40 @@ use datafusion_expr::{
};
use std::sync::Arc;
-/// The rule can be used to the numeric binary comparison with literal expr, like below pattern:
-/// `cast(left_expr as data_type) comparison_op literal_expr` or `literal_expr comparison_op cast(right_expr as data_type)`.
-/// The data type of two sides must be equal, and must be signed numeric type now, and will support more data type later.
+/// [`UnwrapCastInComparison`] Attempts to remove casts from
Review Comment:
Attempts -> attempts?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org