Skip to content

HHH-20262: Spanner PG: Fix Tests Phase 3#11991

Open
sakthivelmanii wants to merge 1 commit intohibernate:mainfrom
sakthivelmanii:fix_spanner_pg_tests_part3
Open

HHH-20262: Spanner PG: Fix Tests Phase 3#11991
sakthivelmanii wants to merge 1 commit intohibernate:mainfrom
sakthivelmanii:fix_spanner_pg_tests_part3

Conversation

@sakthivelmanii
Copy link
Contributor

@sakthivelmanii sakthivelmanii commented Mar 18, 2026

[Please describe here what your change is about]

  1. Added Array and JSON functions
  2. Fixed some of the $ related errors

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.



Please make sure that the following tasks are completed:
Tasks specific to HHH-20262 (Sub-task):

  • Add test OR check there is no need for a test
  • Update documentation as relevant: javadoc for changed API, documentation/src/main/asciidoc/userguide for all features, documentation/src/main/asciidoc/introduction for main features, links from existing documentation
  • Add entries as relevant to migration-guide.adoc (breaking changes) and whats-new.adoc (new features/improvements)

https://hibernate.atlassian.net/browse/HHH-20262

@sakthivelmanii
Copy link
Contributor Author

@beikov This is the last PR for Spanner PG dialect which fixes all the tests.

@RequiresDialectFeature( feature = DialectFeatureChecks.SupportsStructuralArrays.class)
@RequiresDialectFeature( feature = DialectFeatureChecks.SupportsUnnest.class)
@RequiresDialectFeature(feature = DialectFeatureChecks.SupportsJsonAggregate.class)
@SkipForDialect( dialectClass = SpannerPostgreSQLDialect.class, reason = "Spanner PG doesn't support LATERAL JOIN")
Copy link
Member

Choose a reason for hiding this comment

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

Since this seems to depend on json_table to work correctly, I would prefer to see this:

Suggested change
@SkipForDialect( dialectClass = SpannerPostgreSQLDialect.class, reason = "Spanner PG doesn't support LATERAL JOIN")
@RequiresDialectFeature(feature = DialectFeatureChecks.SupportsJsonTable.class)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}

@Test
@RequiresDialectFeature( feature = DialectFeatureChecks.SupportsArrayPosition.class)
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer if you add a new check that checks for definesFunction( dialect, "array_contains_nullable" ).

Suggested change
@RequiresDialectFeature( feature = DialectFeatureChecks.SupportsArrayPosition.class)
@RequiresDialectFeature( feature = DialectFeatureChecks.SupportsArrayContainsNullable.class)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}

@Test
@RequiresDialectFeature( feature = DialectFeatureChecks.SupportsArrayPosition.class)
Copy link
Member

Choose a reason for hiding this comment

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

Again, I would prefer if you add a new check that checks for definesFunction( dialect, "array_contains_nullable" ).

Suggested change
@RequiresDialectFeature( feature = DialectFeatureChecks.SupportsArrayPosition.class)
@RequiresDialectFeature( feature = DialectFeatureChecks.SupportsArrayContainsNullable.class)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

functionFactory.arrayConcat_postgresql();
functionFactory.arrayPrepend_postgresql();
functionFactory.arrayAppend_postgresql();
functionFactory.arrayContains_postgresql();
Copy link
Member

Choose a reason for hiding this comment

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

This is what I mean, i.e. don't register array_contains_nullable since you can't implement it.

Suggested change
functionFactory.arrayContains_postgresql();
functionRegistry.register( "array_contains", new ArrayContainsOperatorFunction( false, typeConfiguration ) );
functionRegistry.register( "array_includes", new ArrayIncludesOperatorFunction( false, typeConfiguration ) );
functionRegistry.register( "array_includes_nullable", new ArrayIncludesOperatorFunction( true, typeConfiguration ) );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines +59 to +60
.filter( collection -> Objects.nonNull( collection.getCollectionTable() ) )
.filter( collection -> collection.getCollectionTable().equals( targetTable ) )
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.filter( collection -> Objects.nonNull( collection.getCollectionTable() ) )
.filter( collection -> collection.getCollectionTable().equals( targetTable ) )
.filter( collection -> collection.getCollectionTable() == targetTable )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

return metadata.getCollectionBindings().stream()
.filter( collection -> Objects.nonNull( collection.getCollectionTable() ) )
.filter( collection -> collection.getCollectionTable().equals( targetTable ) )
.anyMatch( collection -> collection.getElement() instanceof SimpleValue );
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.anyMatch( collection -> collection.getElement() instanceof SimpleValue );
.anyMatch( collection -> collection.getElement() instanceof SimpleValue && !( collection.getElement() instanceof ToOne ));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}

private boolean isHistoryOrAuditedTable(Table table) {
return table.getName().endsWith( "_history" ) || table.getName().endsWith( "_aud" );
Copy link
Member

Choose a reason for hiding this comment

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

It seems to me that this is a safer way to check if a table is an audit table:

Suggested change
return table.getName().endsWith( "_history" ) || table.getName().endsWith( "_aud" );
return metadata.getCollectionBindings().stream()
.anyMatch( collection -> collection.getAuxiliaryTable() == targetTable )
|| metadata.getEntityBindings().stream()
.filter( entity -> entity instanceof RootClass )
.anyMatch( entity -> ((RootClass) entity).getAuxiliaryTable() == targetTable );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines +48 to +63
sqlAppender.appendSql( "jsonb_strip_nulls(jsonb_build_array" );
}
else {
sqlAppender.appendSql( "jsonb_build_array" );
}

char separator = '(';
for ( int i = 0; i < argumentsCount; i++ ) {
sqlAppender.appendSql( separator );
sqlAstArguments.get( i ).accept( walker );
separator = ',';
}
sqlAppender.appendSql( ')' );

if ( nullBehavior == JsonNullBehavior.ABSENT ) {
sqlAppender.appendSql( ')' );
Copy link
Member

Choose a reason for hiding this comment

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

According to the documentation, jsonb_strip_nulls only works on object values, not array values, so you need this instead:

Suggested change
sqlAppender.appendSql( "jsonb_strip_nulls(jsonb_build_array" );
}
else {
sqlAppender.appendSql( "jsonb_build_array" );
}
char separator = '(';
for ( int i = 0; i < argumentsCount; i++ ) {
sqlAppender.appendSql( separator );
sqlAstArguments.get( i ).accept( walker );
separator = ',';
}
sqlAppender.appendSql( ')' );
if ( nullBehavior == JsonNullBehavior.ABSENT ) {
sqlAppender.appendSql( ')' );
sqlAppender.appendSql( "jsonb_path_query_array(jsonb_build_array" );
}
else {
sqlAppender.appendSql( "jsonb_build_array" );
}
char separator = '(';
for ( int i = 0; i < argumentsCount; i++ ) {
sqlAppender.appendSql( separator );
sqlAstArguments.get( i ).accept( walker );
separator = ',';
}
sqlAppender.appendSql( ')' );
if ( nullBehavior == JsonNullBehavior.ABSENT ) {
sqlAppender.appendSql( ",'$[*] ? (@ != null)')" );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, Spanner doesn't support jsonb_path_query_array.

}

@Test
@SkipForDialect(dialectClass = org.hibernate.community.dialect.SpannerPostgreSQLDialect.class, reason = "Spanner PG does not support values in from clause")
Copy link
Member

Choose a reason for hiding this comment

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

After the fix I proposed to SpannerPostgreSQLJsonArrayFunction, you should be able to revert the changes to this file.

@sakthivelmanii sakthivelmanii force-pushed the fix_spanner_pg_tests_part3 branch from 0d5c0a4 to 4c77282 Compare March 18, 2026 17:46
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.

2 participants