Skip to content

Conversation

antiguru
Copy link
Member

@antiguru antiguru commented Oct 7, 2025

Allow constructing multi-dimensional arrays with null values instead of panicking. Similarly to PostgreSQL, treat null elements as zero-dimensional arrays.

Fixes MaterializeInc/database-issues#9757

@antiguru antiguru requested a review from a team as a code owner October 7, 2025 08:41
@antiguru antiguru requested review from ggevay, mgree and teskje and removed request for teskje October 7, 2025 08:41
@mgree
Copy link
Contributor

mgree commented Oct 8, 2025

This should likely come with changes to is_instance_of and is_instance_of_sql (and possibly the Datum Arbitrary instance). (Assuming I understand this as a new behavior correctly. Worth a cross check no matter what!)

Allow constructing multi-dimensional arrays with null values instead of
panicking. Similarly to PostgreSQL, treat null elements as zero-dimensional
arrays.

Fixes MaterializeInc/database-issues#9757

Signed-off-by: Moritz Hoffmann <[email protected]>
@antiguru antiguru force-pushed the fix_9757 branch 2 times, most recently from 51d12bf to 7d71bbc Compare October 9, 2025 14:26
Signed-off-by: Moritz Hoffmann <[email protected]>
@antiguru
Copy link
Member Author

antiguru commented Oct 9, 2025

This should likely come with changes to is_instance_of and is_instance_of_sql (and possibly the Datum Arbitrary instance). (Assuming I understand this as a new behavior correctly. Worth a cross check no matter what!)

I understand but I don't know what needs to be true for an array datum to be instances of another a specific array type. Do we need to consider the dimensions?

let actual = a.length;
let expected = e.length;
// All input arrays must have the same dimensionality.
return Err(InvalidArrayError::WrongCardinality { actual, expected }.into());
Copy link
Member Author

Choose a reason for hiding this comment

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

We might need another error variant to be precise, but I feel we're good with the current one being used in two different contexts (length vs. dims).

Copy link
Contributor

Choose a reason for hiding this comment

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

Postgres also checks for the lower bound being the same, even if the lengths are the same and it also produces a more generic error message than us:

postgres=# SELECT ARRAY['[0:0]={1}'::int[], '[1:1]={2}'::int[]];
ERROR:  multidimensional arrays must have array expressions with matching dimensions

What do you think of changing our error variant to give the same generic error message and then here we just do a simple if actual_dims != expected_dims { ... } ?

@mgree
Copy link
Contributor

mgree commented Oct 15, 2025

This should likely come with changes to is_instance_of and is_instance_of_sql (and possibly the Datum Arbitrary instance). (Assuming I understand this as a new behavior correctly. Worth a cross check no matter what!)

I understand but I don't know what needs to be true for an array datum to be instances of another a specific array type. Do we need to consider the dimensions?

See is_instance_of_sql:

(Datum::Array(array), SqlScalarType::Array(t)) => {
array.elements.iter().all(|e| match e {
Datum::Null => true,
_ => is_instance_of_scalar(e, t),
})
}
(Datum::Array(array), SqlScalarType::Int2Vector) => {
array.dims().len() == 1
&& array
.elements
.iter()
.all(|e| is_instance_of_scalar(e, &SqlScalarType::Int16))
}
(Datum::Array(_), _) => false,

It seems the dimensions don't matter (unless you're dealing with int2vec weirdness).

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.

3 participants