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

Fix Array Containment Operator Mapping and pgcompat Issue #28044

Merged
merged 11 commits into from
Jul 8, 2024

Conversation

sthm
Copy link
Contributor

@sthm sthm commented Jul 4, 2024

closes #28035

Copy link
Contributor

@nrainer-materialize nrainer-materialize left a comment

Choose a reason for hiding this comment

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

Thanks for the fix and additional test cases!

@sthm sthm requested a review from ParkMyCar July 4, 2024 12:38
@sthm sthm marked this pull request as ready for review July 4, 2024 12:39
@sthm sthm requested a review from a team as a code owner July 4, 2024 12:39
@nrainer-materialize
Copy link
Contributor

Rebased on main and added 00a6723, which reverts 533aba1.

@nrainer-materialize
Copy link
Contributor

Nightly build for the re-enabled contains operator in the output-consistency framework: https://buildkite.com/materialize/nightly/builds/8366

@sthm
Copy link
Contributor Author

sthm commented Jul 7, 2024

This is a minimal repro which returns true with the current implementation but false in Postgres.

SELECT '{NULL}'::numeric[] @> '{NULL}'::numeric[];

I suspect this happens because the implementation is checking NULL == NULL in Rust, which returns true.

@sthm
Copy link
Contributor Author

sthm commented Jul 8, 2024

Nightly is still hitting this panic:

2024-07-08 09:54:12 thread 'timely:work-1' panicked at src/expr/src/scalar/func.rs:1630:15:
2024-07-08 09:54:12 Datum::unwrap_list called on Array(Array { elements: [String("a"), String("b"), String("a"), String("b")], dims: [ArrayDimension { lower_bound: 1, length: 2 }, ArrayDimension { lower_bound: 1, length: 2 }] })

@sthm
Copy link
Contributor Author

sthm commented Jul 8, 2024

For some reason, the panic does not reproduce in a local build.

localhost > SELECT ((multi_dim_text_array_one_elem || multi_dim_text_array_one_elem) @> multi_dim_text_array_empty), (text_val_3 !~* '.*'), ((timestamp_max_val - time_max_val))::TEXT FROM t_dfr_2_v0_108_0_dev_horiz;
 ?column? | ?column? |            text
----------+----------+-----------------------------
 t        | f        | 99999-12-30 23:59:59.000001
(1 row)

with

localhost > SELECT multi_dim_text_array_one_elem, multi_dim_text_array_empty, text_val_3, timestamp_max_val, time_max_val FROM t_dfr_2_v0_108_0_dev_horiz;
 multi_dim_text_array_one_elem | multi_dim_text_array_empty | text_val_3 |  timestamp_max_val   |  time_max_val
-------------------------------+----------------------------+------------+----------------------+-----------------
 {{a,b}}                       | {}                         | xAAx       | 99999-12-31 23:59:59 | 23:59:59.999999
(1 row)

@nrainer-materialize
Copy link
Contributor

For some reason, the panic does not reproduce in a local build.

localhost > SELECT ((multi_dim_text_array_one_elem || multi_dim_text_array_one_elem) @> multi_dim_text_array_empty), (text_val_3 !~* '.*'), ((timestamp_max_val - time_max_val))::TEXT FROM t_dfr_2_v0_108_0_dev_horiz;
 ?column? | ?column? |            text
----------+----------+-----------------------------
 t        | f        | 99999-12-30 23:59:59.000001
(1 row)

with

localhost > SELECT multi_dim_text_array_one_elem, multi_dim_text_array_empty, text_val_3, timestamp_max_val, time_max_val FROM t_dfr_2_v0_108_0_dev_horiz;
 multi_dim_text_array_one_elem | multi_dim_text_array_empty | text_val_3 |  timestamp_max_val   |  time_max_val
-------------------------------+----------------------------+------------+----------------------+-----------------
 {{a,b}}                       | {}                         | xAAx       | 99999-12-31 23:59:59 | 23:59:59.999999
(1 row)

The failure in the nightly build occurs in the version-consistency build step. This build step compares the current version against its ancestor. In this case, the current version will be compared against the merge base, which is the commit from which this branch branched off. Since that earlier version contained the fault causing a panic, it is expected for this build to fail (it exercises the earlier version as well).
On main, the comparison will be conducted against the previous release. Therefore, there should be no build failure if the fix manages to get into the same release as the fault. (Otherwise, we have means to address this if we want to.)
As discussed with @sthm, the feature was introduced in v0.107.0. I suggest to backport the fix to v0.107.x. Failures of version-consistency on builds of v0.107.1+ are to be expected. I expect no failures on builds of main in this case.

=> TLDR: Failure of version-consistency is expected. Good from QA side.

@sthm sthm changed the title Fix array containment operator that was mapped to list containment Fix Array Containment Operator Mapping and pgcompat Issue Jul 8, 2024
@ParkMyCar ParkMyCar merged commit 688620c into MaterializeInc:main Jul 8, 2024
74 checks passed
ParkMyCar pushed a commit that referenced this pull request Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

panic: Datum::unwrap_list called on Array when using contains operator
3 participants