Skip to content

Conversation

@labkey-matthewb
Copy link
Contributor

Rationale

SQL Array methods to support multi choice fields. Dialect methods for use by LabKey SQL as well as dataregion filters (NYI).

Related Pull Requests

Changes

checkpoint
SELECT 'a' as test, array_contains_all(  ARRAY['A','X'], ARRAY['A','B'] ) as result
UNION ALL
SELECT 'b' as test, array_contains_any(  ARRAY['A','X'], ARRAY['A','B'] ) as result
UNION ALL
SELECT 'c' as test, array_contains_none( ARRAY['A','X'], ARRAY['A','B'] ) as result
UNION ALL
SELECT 'd' as test, array_is_same(       ARRAY['A','X'], ARRAY['A','B'] ) as result
UNION ALL
SELECT 'd' as test, array_is_same(       ARRAY['A','B'], ARRAY['A','B'] ) as result
@labkey-matthewb labkey-matthewb requested a review from XingY December 3, 2025 20:23
for (SQLFragment element : elements)
{
ret.append(separator);
ret.append(element);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there special encoding needed? For example a'b needs to be encoded into a''b?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any encoding should be handled by whoever generated the elements. Those should be correct SQL already.

ret.append(element);
separator = ", ";
}
ret.append("]");
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we always assume text[] type? I don't think this works for varchar[]. If the column is of type varchar[], the column needs to be casted to text[] first for the comparators to work:
SELECT * from vehicle.colors where tagsvarchar::text[] @> ARRAY['pastel', 'primary'];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question... We assume that any array we create would be text, and I think we an require that for module created tables as well. It would be tricky for us to handle varchar (say for attached schemas). Currently we don't track varchar and text separately they tend to just get merged as JdbcType.VARCHAR.

Worth adding a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder what happens when using VARCHAR columns e.g

tagsvarchar::text[] @> ARRAY[varcharColumn1, varcharColumn2'];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we need a TEXTARRAY() method? I'll think about it.

{
// (ELEMENT...)
postgresMethods.put("array_construct", new ArrayConstructMethod("array_construct"));

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be useful to have a method that returns the number of elements in the array. Or a method that checks if array contains >1 elements. This would be useful for the story that support converting between single and multi value text choices.

@labkey-matthewb labkey-matthewb requested a review from XingY December 8, 2025 18:58
@Override
public SQLFragment array_none_in_array(SQLFragment a, SQLFragment b)
{
return new SQLFragment(" NOT (").append(array_some_in_array(a, b)).append(")");
Copy link
Contributor

Choose a reason for hiding this comment

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

Should array_none_in_array also return empty values?


// not array_equals() because arrays are ordered, this is an unordered comparison
postgresMethods.put("array_is_same", new ArrayOperatorMethod("array_is_same", SqlDialect::array_same_array));
// Use "NOT array_is_same()" instead of something clumsy like "array_is_not_same()"
Copy link
Contributor

Choose a reason for hiding this comment

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

All of those methods only return records that are not empty. However, NOT array_op also only return non empty rows. I would think "Does Not include Any Of" should also return empty reocrds.

The spec calls for support for "Is Blank" and "Is not blank" methods. Maybe we should also have at least a array_is_empty method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For VARCHAR we basically said we don't want both NULL and '', it's too confusing, so we always convert ''->NULL. I can see that NULL and [] are also super confusing. Perhaps we could always convert ''/blank/null->[] in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the array_is_empty() idea.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request introduces SQL array methods to support multi-choice fields in LabKey SQL and data region filters. The implementation adds dialect-specific methods for array operations, primarily targeting PostgreSQL.

Key Changes:

  • Added array literal syntax support (ARRAY[...] and TEXTARRAY[...]) to the SQL grammar
  • Implemented array operation methods (contains, containment checking, equality comparison) in SqlDialect with PostgreSQL-specific implementations
  • Added comprehensive unit tests for array operations covering various edge cases including NULL handling

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
query/src/org/labkey/query/sql/SqlBase.g Added ARRAY and TEXTARRAY tokens and grammar rules for array literal syntax
query/src/org/labkey/query/sql/Method.java Introduced ArrayOperatorMethod, ArrayConstructMethod, and TextArrayConstructMethod classes; registered array methods for PostgreSQL; applied code formatting improvements
query/src/org/labkey/query/QueryTestCase.jsp Added comprehensive unit tests for dialect array methods and SQL array operations
core/src/org/labkey/core/dialect/PostgreSql92Dialect.java Implemented PostgreSQL-specific array operations using native operators (<@, &&, ANY, ALL)
api/src/org/labkey/api/data/dialect/SqlDialect.java Added abstract array support methods to base SqlDialect class

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +1642 to +1646
postgresMethods.put("array_contains_element", new ArrayOperatorMethod("array_contains_element", (d,a,b) -> d.element_in_array(b,a)));
// Use "NOT array_contains_element()" instead of something clumsy like "array_does_not_contain()"

// (ARRAY, ARRAY)
postgresMethods.put("array_contains_all", new ArrayOperatorMethod("array_contains_all", (d,a,b) -> d.array_all_in_array(b,a)));
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

The lambda expressions on lines 1642, 1646 have inconsistent spacing. Line 1642 has (d,a,b) with no spaces after commas, while line 1647-1648 follow the file's convention with proper spacing. Add spaces after commas: (d, a, b)

Suggested change
postgresMethods.put("array_contains_element", new ArrayOperatorMethod("array_contains_element", (d,a,b) -> d.element_in_array(b,a)));
// Use "NOT array_contains_element()" instead of something clumsy like "array_does_not_contain()"
// (ARRAY, ARRAY)
postgresMethods.put("array_contains_all", new ArrayOperatorMethod("array_contains_all", (d,a,b) -> d.array_all_in_array(b,a)));
postgresMethods.put("array_contains_element", new ArrayOperatorMethod("array_contains_element", (d, a, b) -> d.element_in_array(b, a)));
// Use "NOT array_contains_element()" instead of something clumsy like "array_does_not_contain()"
// (ARRAY, ARRAY)
postgresMethods.put("array_contains_all", new ArrayOperatorMethod("array_contains_all", (d, a, b) -> d.array_all_in_array(b, a)));

Copilot uses AI. Check for mistakes.
Comment on lines +2147 to +2203
// construct a sql array from SQLFragment elements
public SQLFragment array_construct(SQLFragment[] elements)
{
assert !supportsArrays();
throw new UnsupportedOperationException(getClass().getSimpleName() + " does not implement");
}

// element a is in array b
public SQLFragment element_in_array(SQLFragment a, SQLFragment b)
{
assert !supportsArrays();
throw new UnsupportedOperationException(getClass().getSimpleName() + " does not implement");
}

// element a is not in array b
public SQLFragment element_not_in_array(SQLFragment a, SQLFragment b)
{
assert !supportsArrays();
throw new UnsupportedOperationException(getClass().getSimpleName() + " does not implement");
}

// SET OPERATORS FOR ARRAY TYPE

// all elements of array a are contained in array b
public SQLFragment array_all_in_array(SQLFragment a, SQLFragment b)
{
assert !supportsArrays();
throw new UnsupportedOperationException(getClass().getSimpleName() + " does not implement");
}

// some elements of array a are contained in array b
public SQLFragment array_some_in_array(SQLFragment a, SQLFragment b)
{
assert !supportsArrays();
throw new UnsupportedOperationException(getClass().getSimpleName() + " does not implement");
}

// no elements of array a are contained in array b
public SQLFragment array_none_in_array(SQLFragment a, SQLFragment b)
{
assert !supportsArrays();
throw new UnsupportedOperationException(getClass().getSimpleName() + " does not implement");
}

// arrays a and b contain the same elements equivalent to (A all in B) AND (B all in A)
public SQLFragment array_same_array(SQLFragment a, SQLFragment b)
{
assert !supportsArrays();
throw new UnsupportedOperationException(getClass().getSimpleName() + " does not implement");
}

// array a and array b do not contain the same elements
public SQLFragment array_not_same_array(SQLFragment a, SQLFragment b)
{
assert !supportsArrays();
throw new UnsupportedOperationException(getClass().getSimpleName() + " does not implement");
}
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

The comments for these public API methods should use proper Javadoc format with /** instead of // for better documentation generation. Consider using proper Javadoc like:

/**
 * Construct a SQL array from SQLFragment elements.
 * @param elements The elements to include in the array
 * @return SQLFragment representing the array construction
 */

Copilot uses AI. Check for mistakes.
labkey-matthewb and others added 2 commits December 9, 2025 20:49
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@labkey-matthewb labkey-matthewb merged commit a84e761 into develop Dec 11, 2025
12 of 14 checks passed
@labkey-matthewb labkey-matthewb deleted the fb_dialect_arrays branch December 11, 2025 19:48
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