feat(frontend): add shrink cast for condition.split_to_scan_ranges() #7962

ZENOTME · 2023-02-16T05:19:43Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

assign type suffix to smallint,int,bigint to solve bug: prepared statement ignores explicit types for actual query results #7900
add shrink cast which can cast a bigger range type to smaller range type, e.g.: bigint -> int

related issue: #7916

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features).
I have demonstrated that backward compatibility is not broken by breaking changes and created issues to track deprecated features to be removed in the future. (Please refer to the issue)
All checks passed in ./risedev check (or alias, ./risedev c)

Documentation

My PR DOES NOT contain user-facing changes.

Click here for Documentation

Types of user-facing changes

Please keep the types that apply to your changes, and remove the others.

Installation and deployment
Connector (sources & sinks)
SQL commands, functions, and operators
RisingWave cluster configuration changes
Other (please specify in the release note below)

Release note

ZENOTME · 2023-02-16T05:22:23Z

Do u think this implementation works? @xiangjinwu @chenzl25 PTAL

If it works, I can complete the rest todo in this PR or later PR.

TODO:

support shrink cast in as_in_const_list
support more type shrink cast

chenzl25

LGTM. @xiangjinwu PTAL. I am not quite sure whether it is the best way to deal with the implicit cast and 'shrink' cast.

jon-chuang · 2023-02-20T03:54:59Z

src/frontend/src/expr/mod.rs

+            _ => return Ok(CastResult::Failed),
+        };
+        if let Some(scalar) = self.eval_row_const()? {
+            let value = scalar.as_integral();


Hmm, will we be able to handle float, decimal as well?

Good question... So it is not enough to only check for minimum and maximum. For where id < 2.5 and where id > 2.5, we need to round in different directions: ceil to id < 3 and floor to id > 2. And when it is id = 2.5, this should evaluate to false. And where id <= 2.5 to where id <= 2, where id >= 2.5 to where id >= 3.

There may be more complicated edge cases we are missing. Without optimization, the system always casts to a "larger" type and compare in that type. But in this optimization, we always want to compare in the type of order column, which may not be the "larger" type. Mimicking this CMP_B(A_TO_B(x), A_TO_B(y)) via CMP_A(B_TO_A(x), B_TO_A(y)) requires us to find the proper B_TO_A function given existing A_TO_B, CMP_B and CMP_A for all possible inputs.

More edge cases (PostgreSQL):

test=# select bigint '9223372036854775807' = double precision '9223372036854775296'; ?column? ---------- t (1 row) test=# select (double precision '9223372036854775296')::bigint; ERROR: bigint out of range test=# select (double precision '9223372036854775295')::bigint; int8 --------------------- 9223372036854774784 (1 row)

Given it is an optimization, my suggestions is just to do it with best effort, and remain unoptimized for cases hard to handle correctly. int16/int32/int64 may be a good subset.

Also given its best-effort nature, the "cast_shrink" series helper functions should be limited to a mod for this special optimization, rather than on ExprImpl.

jon-chuang · 2023-02-20T03:56:46Z

src/frontend/src/expr/mod.rs

    /// Shorthand to create cast expr to `target` type in implicit context.
-    pub fn cast_implicit(self, target: DataType) -> Result<ExprImpl> {
+    pub fn cast_implicit(self, target: DataType) -> std::result::Result<ExprImpl, ErrorCode> {


Hmm, why is it necessary to change the error type?

For RwError, it will capture the backtrace so the cost of constructing the RwError is high. There is a known issue #6131.

In cast_implicit_or_shrink, We will store Error return form cast_implicit before call cast_shrink, so if cast_implicit return a RwError, the performance is unacceptable.

xiangjinwu · 2023-02-23T05:41:07Z

src/frontend/src/utils/condition.rs

+            _ => Err(ErrorCode::BindError(format!(
+                "Cannot cast type \"{}\" to \"{}\".",
+                const_expr.return_type(),
+                target
+            ))


This error is now thrown all the way up to the user:

dev=> create table t (v1 int primary key); CREATE_TABLE dev=> select * from t where v1 < 1.5; ERROR: QueryError: Bind error: Cannot cast type "numeric" to "integer".

But it should just be a signal to skip the scan range optimization. Talking about its caller analyze_eq_const_expr behavior, it should do one of the 3 things:

return Ok(true) so that the upper layer return Ok(false_cond())

assign eq_conds and return Ok(false)

signal the upper layer it cannot match a known optimization and it should just other_conds.push(expr);

We are lacking the last possibility right now.

ZENOTME · 2023-02-24T07:00:23Z

Summarize the todo work:

handle float, decimal and more type
there are some todo work in split_to_scan_range, like merge upperbound and lowerbound

For these todo work, I add the related plan test named Cannot do sth and I will fix them in later PR.

codecov · 2023-02-24T07:18:19Z

Codecov Report

Merging #7962 (588ddac) into main (9db3773) will increase coverage by 0.00%.
The diff coverage is 87.09%.

@@           Coverage Diff           @@
##             main    #7962   +/-   ##
=======================================
  Coverage   71.57%   71.58%           
=======================================
  Files        1132     1132           
  Lines      182874   182962   +88     
=======================================
+ Hits       130897   130976   +79     
- Misses      51977    51986    +9

Flag	Coverage Δ
rust	`71.58% <87.09%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/common/src/types/mod.rs	`73.31% <50.00%> (-0.25%)`	⬇️
src/frontend/src/utils/condition.rs	`95.19% <87.50%> (-1.21%)`	⬇️
src/utils/pgwire/src/pg_extended.rs	`90.90% <100.00%> (+0.03%)`	⬆️
src/storage/src/hummock/sstable/xor_filter.rs	`97.53% <0.00%> (-1.24%)`	⬇️
src/batch/src/task/task_execution.rs	`51.62% <0.00%> (-0.51%)`	⬇️
src/common/src/types/ordered_float.rs	`30.87% <0.00%> (-0.20%)`	⬇️
src/storage/src/hummock/compactor/mod.rs	`80.38% <0.00%> (-0.20%)`	⬇️
src/batch/src/executor/group_top_n.rs	`74.70% <0.00%> (+6.47%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

xiangjinwu

The current implementation looks correct, but I would like to try harder and see if the code structure, function names and doc comments can be easier to understand 🤯

src/frontend/planner_test/tests/testdata/range_scan.yaml

ZENOTME · 2023-02-27T11:39:50Z

The interface and process before is indeed complex.😭 I have try to make it clear. Please let me know if there are still change needed.

xiangjinwu

LGTM. Thanks for the refactor!

src/frontend/src/utils/condition.rs

xiangjinwu · 2023-02-28T05:51:20Z

src/frontend/src/utils/condition.rs

+                    };
+
+                    let Some(new_cond) = new_expr.eval_row_const()? else {
+                        // column = NULL, PK column never be NULL


FYI even NULL = NULL is not true. The result is NULL. Anything = NULL is always NULL.

src/frontend/src/utils/condition.rs

mergify · 2023-02-28T09:18:08Z

Hey @ZENOTME, this pull request failed to merge and has been dequeued from the merge train. If you believe your PR failed in the merge train because of a flaky test, requeue it by clicking "Update branch" or pushing an empty commit with git commit --allow-empty -m "rerun" && git push.

…7962) * specify integral type in pgwire-extended * support mismatch process * support more plan test * refine implementation * refine plan test * refine implementation --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

github-actions bot added the type/feature label Feb 16, 2023

ZENOTME requested review from chenzl25 and xiangjinwu February 16, 2023 05:51

ZENOTME marked this pull request as ready for review February 16, 2023 07:18

chenzl25 approved these changes Feb 17, 2023

View reviewed changes

jon-chuang reviewed Feb 20, 2023

View reviewed changes

ZENOTME force-pushed the zj/shrink_cast branch from dc25f4a to 77dd7c4 Compare February 22, 2023 14:38

xiangjinwu reviewed Feb 23, 2023

View reviewed changes

ZENOTME added 2 commits February 24, 2023 14:44

specify integral type in pgwire-extended

741e359

support mismatch process

a4d0221

ZENOTME force-pushed the zj/shrink_cast branch from 77dd7c4 to dcdf1e2 Compare February 24, 2023 06:56

ZENOTME requested a review from xiangjinwu February 24, 2023 07:21

xiangjinwu reviewed Feb 27, 2023

View reviewed changes

src/frontend/planner_test/tests/testdata/range_scan.yaml Outdated Show resolved Hide resolved

ZENOTME added 2 commits February 27, 2023 17:04

support more plan test

7986488

refine implementation

4b46b98

ZENOTME force-pushed the zj/shrink_cast branch from dcdf1e2 to 4b46b98 Compare February 27, 2023 11:26

refine plan test

ef8dec9

xiangjinwu approved these changes Feb 28, 2023

View reviewed changes

ZENOTME force-pushed the zj/shrink_cast branch from 3f70dbd to bf41eea Compare February 28, 2023 08:04

refine implementation

269912a

ZENOTME force-pushed the zj/shrink_cast branch from bf41eea to 269912a Compare February 28, 2023 08:08

ZENOTME added the mergify/can-merge label Feb 28, 2023

mergify bot added 2 commits February 28, 2023 08:32

Merge branch 'main' into zj/shrink_cast

b89dadc

Merge branch 'main' into zj/shrink_cast

64bc128

xiangjinwu enabled auto-merge February 28, 2023 08:50

Merge branch 'main' into zj/shrink_cast

588ddac

xiangjinwu added this pull request to the merge queue Feb 28, 2023

xiangjinwu removed the mergify/can-merge label Feb 28, 2023

Merged via the queue into main with commit c0b48bf Feb 28, 2023

xiangjinwu deleted the zj/shrink_cast branch February 28, 2023 09:34

ZENOTME linked an issue Mar 3, 2023 that may be closed by this pull request

bug: prepared statement ignores explicit types for actual query results #7900

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(frontend): add shrink cast for condition.split_to_scan_ranges() #7962

feat(frontend): add shrink cast for condition.split_to_scan_ranges() #7962

ZENOTME commented Feb 16, 2023 •

edited

Loading

ZENOTME commented Feb 16, 2023 •

edited

Loading

chenzl25 left a comment

jon-chuang Feb 20, 2023

ZENOTME Feb 20, 2023

xiangjinwu Feb 20, 2023 •

edited

Loading

jon-chuang Feb 20, 2023

ZENOTME Feb 20, 2023

xiangjinwu Feb 23, 2023

ZENOTME commented Feb 24, 2023

codecov bot commented Feb 24, 2023 •

edited

Loading

xiangjinwu left a comment

ZENOTME commented Feb 27, 2023

xiangjinwu left a comment

xiangjinwu Feb 28, 2023

mergify bot commented Feb 28, 2023

feat(frontend): add shrink cast for condition.split_to_scan_ranges() #7962

feat(frontend): add shrink cast for condition.split_to_scan_ranges() #7962

Conversation

ZENOTME commented Feb 16, 2023 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Types of user-facing changes

Release note

ZENOTME commented Feb 16, 2023 • edited Loading

chenzl25 left a comment

Choose a reason for hiding this comment

jon-chuang Feb 20, 2023

Choose a reason for hiding this comment

ZENOTME Feb 20, 2023

Choose a reason for hiding this comment

xiangjinwu Feb 20, 2023 • edited Loading

Choose a reason for hiding this comment

jon-chuang Feb 20, 2023

Choose a reason for hiding this comment

ZENOTME Feb 20, 2023

Choose a reason for hiding this comment

xiangjinwu Feb 23, 2023

Choose a reason for hiding this comment

ZENOTME commented Feb 24, 2023

codecov bot commented Feb 24, 2023 • edited Loading

Codecov Report

xiangjinwu left a comment

Choose a reason for hiding this comment

ZENOTME commented Feb 27, 2023

xiangjinwu left a comment

Choose a reason for hiding this comment

xiangjinwu Feb 28, 2023

Choose a reason for hiding this comment

mergify bot commented Feb 28, 2023

ZENOTME commented Feb 16, 2023 •

edited

Loading

ZENOTME commented Feb 16, 2023 •

edited

Loading

xiangjinwu Feb 20, 2023 •

edited

Loading

codecov bot commented Feb 24, 2023 •

edited

Loading