SQL vector support across JDBC/R2DBC with dialect-aware scoring#3637
SQL vector support across JDBC/R2DBC with dialect-aware scoring#3637
Conversation
data-jdbc/src/main/java/io/micronaut/data/jdbc/config/SchemaGenerator.java
Outdated
Show resolved
Hide resolved
...c/test/groovy/io/micronaut/data/jdbc/oraclexe/vector/OracleJdbcDoubleVectorEntitySpec.groovy
Show resolved
Hide resolved
...o/micronaut/data/runtime/operations/internal/query/DefaultBindableParametersStoredQuery.java
Outdated
Show resolved
Hide resolved
|
Can you try to implement it without changes in DataType and get/set mapping? The idea would be to use DataType.OBJECT and use a custom The idea would be to implement it in almost non-invasive way so other things like geo etc can be supported as well easily. |
data-jdbc/src/main/java/io/micronaut/data/jdbc/operations/DefaultJdbcRepositoryOperations.java
Outdated
Show resolved
Hide resolved
data-jdbc/src/main/java/io/micronaut/data/jdbc/operations/DefaultJdbcRepositoryOperations.java
Outdated
Show resolved
Hide resolved
data-jdbc/src/main/java/io/micronaut/data/jdbc/operations/DefaultJdbcRepositoryOperations.java
Outdated
Show resolved
Hide resolved
data-model/src/main/java/io/micronaut/data/model/runtime/RuntimePersistentProperty.java
Outdated
Show resolved
Hide resolved
data-model/src/main/java/io/micronaut/data/model/runtime/convert/AttributeConverter.java
Outdated
Show resolved
Hide resolved
data-runtime/src/main/java/io/micronaut/data/runtime/mapper/sql/SqlResultEntityTypeMapper.java
Outdated
Show resolved
Hide resolved
data-model/src/main/java/io/micronaut/data/model/vector/DoubleVector.java
Outdated
Show resolved
Hide resolved
data-model/src/main/java/io/micronaut/data/model/vector/FloatVector.java
Outdated
Show resolved
Hide resolved
data-model/src/main/java/io/micronaut/data/model/vector/ByteVector.java
Outdated
Show resolved
Hide resolved
data-model/src/main/java/io/micronaut/data/model/vector/ByteVector.java
Outdated
Show resolved
Hide resolved
data-model/src/main/java/io/micronaut/data/model/vector/DoubleVector.java
Outdated
Show resolved
Hide resolved
data-model/src/main/java/io/micronaut/data/model/vector/IntVector.java
Outdated
Show resolved
Hide resolved
data-model/src/main/java/io/micronaut/data/model/vector/IntVector.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 205 out of 223 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
data-r2dbc/src/main/java/io/micronaut/data/r2dbc/config/R2dbcSchemaGenerator.java:148
generate()logs every CREATE TABLE statement at WARN level (LOG.warn("Create table..." )), which will spam logs during normal application startup/tests even when nothing is wrong. Please downgrade this to DEBUG/TRACE (or remove it) and rely on the existingDataSettings.QUERY_LOGdebug logging for visibility.
data-r2dbc/src/main/java/io/micronaut/data/r2dbc/operations/OracleR2dbcVectorBindSupport.java
Outdated
Show resolved
Hide resolved
...o/micronaut/data/runtime/operations/internal/query/DefaultBindableParametersStoredQuery.java
Show resolved
Hide resolved
data-jdbc/src/test/groovy/io/micronaut/data/jdbc/h2/many2many/ManyToManyJoinTableSpec.groovy
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,31 @@ | |||
| /* | |||
| * Copyright 2017-2021 original authors | |||
There was a problem hiding this comment.
Maybe could update year since it may be confusing since it is since 5.0.x (there are other classes in the package to be updated)
| public record FloatVector(float[] data) implements Vector { | ||
|
|
||
|
|
||
| /** | ||
| * Creates a float vector. | ||
| * | ||
| * @param data the backing values | ||
| */ | ||
| public FloatVector { | ||
| Objects.requireNonNull(data, "FloatVector data must not be null"); | ||
| } |
There was a problem hiding this comment.
These vector records expose the backing array directly via the record component accessor (data()), and the canonical constructor does not defensively copy the input array. That makes the type effectively mutable (callers can mutate the array after construction, or through data()), breaking the "immutable vector" contract and potentially invalidating equals/hashCode. Consider defensively copying in the canonical constructor and overriding data() to return a copy (or avoid using a record for array-backed immutables).
| public record DoubleVector(double[] data) implements Vector { | ||
|
|
||
|
|
||
| /** | ||
| * Creates a double vector. | ||
| * | ||
| * @param data the backing values | ||
| */ | ||
| public DoubleVector { | ||
| Objects.requireNonNull(data, "DoubleVector data must not be null"); | ||
| } |
There was a problem hiding this comment.
DoubleVector has the same mutability issue as FloatVector: the record component accessor returns the internal double[] and the constructor doesn't copy the input array. This allows external mutation and can break the immutability and equals/hashCode expectations. Defensively copy in the constructor and override data() to return a copy (or switch away from records for array-backed values).
| public record ByteVector(byte[] data) implements Vector { | ||
|
|
||
|
|
||
| /** | ||
| * Creates a byte vector. | ||
| * | ||
| * @param data the backing values | ||
| */ | ||
| public ByteVector { | ||
| Objects.requireNonNull(data, "ByteVector data must not be null"); | ||
| } |
There was a problem hiding this comment.
ByteVector is declared as immutable but, as a record, it exposes its internal byte[] via the generated data() accessor and does not defensively copy the array in the constructor. Callers can mutate the backing storage, which can break equality/hashCode and lead to subtle bugs. Defensively copy the array on construction and override data() to return a copy (or avoid using a record for array-backed types).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 205 out of 223 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (7)
data-r2dbc/build.gradle:1
- The dependency alias
mnSql.mysql.connector.jlooks truncated and is very likely not defined in the version catalog (previouslymnSql.mysql.connector.java). If the alias doesn't exist, this will break Gradle resolution across multiple modules—please restore the correct alias or add/rename the catalog entry consistently.
data-r2dbc/build.gradle:1 - The dependency alias
mnSql.mysql.connector.jlooks truncated and is very likely not defined in the version catalog (previouslymnSql.mysql.connector.java). If the alias doesn't exist, this will break Gradle resolution across multiple modules—please restore the correct alias or add/rename the catalog entry consistently.
doc-examples/r2dbc-example-kotlin/src/main/resources/application.yml:1 - The container tag
container-registry.oracle.com/mysql/community-server:9.6.0is likely not a valid MySQL Server version/tag and may fail pulls in CI. Consider using a known published MySQL tag (e.g., an 8.4.x LTS tag) or the existingmysql:<version>image used elsewhere, and avoid registries that require authentication unless that’s explicitly handled.
data-runtime/src/main/java/io/micronaut/data/runtime/operations/internal/sql/SqlJsonColumnMapperProvider.java:1 - This changes both the constructor signature and reduces its visibility (from
publicto package-private). Since the class itself ispublic, this is a source/binary compatibility break for any external code constructing it. If the intent is DI-only wiring, keep the constructorpublic(or add apublicdelegating constructor overload) while adding the new dependency.
data-processor/src/main/java/io/micronaut/data/processor/visitors/finders/FindersUtils.java:1 SearchResults<T>represents a multi-row result container, but this branch routes it throughFindOne*interceptors (sync/reactive/async). That is very likely to only read/map a single row and silently drop the rest. A safer approach is to routeSearchResultsthrough an interceptor that iterates all rows (similar toFindAll*) and then wraps them intoSearchResults.
data-runtime/src/main/java/io/micronaut/data/runtime/operations/internal/query/DefaultBindableParametersStoredQuery.java:1- This uses Java record deconstruction patterns (
value instanceof Score(double score)/Similarity(double similarity)), which require Java 21+. If the project/toolchain baseline is below 21, this will not compile; use standardinstanceof Score scoreandscore.value()accessors instead (or ensure the build enforces Java 21+).
data-r2dbc/src/main/java/io/micronaut/data/r2dbc/mapper/ColumnNameR2dbcResultReader.java:1 getRequiredValuenow returnsnullwhen conversion is not possible (convert(...).orElse(null)), which can mask type/codec issues and violates the usual expectation implied by 'Required'. Consider throwing aDataAccessExceptionwhen conversion returns empty (or using aconvertRequired-style path) so mapping failures are surfaced deterministically.
data-model/src/main/java/io/micronaut/data/model/vector/FloatVector.java
Show resolved
Hide resolved
data-model/src/main/java/io/micronaut/data/model/vector/DoubleVector.java
Show resolved
Hide resolved
data-runtime/src/main/java/io/micronaut/data/runtime/query/internal/DefaultPreparedQuery.java
Show resolved
Hide resolved
|
|
||
| // LocalDate | ||
| conversionService.addConverter(LocalDate.class, java.sql.Date.class, java.sql.Date::valueOf); | ||
| conversionService.addConverter(LocalDate.class, Date.class, localDate -> Date.from(localDate.atStartOfDay(ZoneId.systemDefault()).toInstant())); | ||
|
|
There was a problem hiding this comment.
There is already a ChronoLocalDate -> java.util.Date converter registered earlier in this factory. Adding a second LocalDate -> Date converter is redundant (LocalDate implements ChronoLocalDate) and can make converter selection harder to reason about. Prefer relying on the existing ChronoLocalDate converter, or document why the more specific converter is necessary.
data-r2dbc/src/main/java/io/micronaut/data/r2dbc/mapper/ColumnNameR2dbcResultReader.java
Show resolved
Hide resolved
| // Ensure pgvector extension and demo table for vector tests | ||
| try (CallableStatement st = connection.prepareCall("CREATE EXTENSION IF NOT EXISTS vector;")) { | ||
| st.execute(); | ||
| } catch (SQLException e) { | ||
| // Ignore if not available or already exists | ||
| } |
There was a problem hiding this comment.
This initializer is meant to ensure the vector extension exists for vector specs, but it currently swallows any SQLException from CREATE EXTENSION .... If the extension is missing or permissions prevent creation, tests will fail later with harder-to-diagnose errors. Consider letting the exception propagate (or at least logging) for this vector-specific init path.
data-r2dbc/src/main/java/io/micronaut/data/r2dbc/mapper/ColumnNameR2dbcResultReader.java
Show resolved
Hide resolved
|



Uh oh!
There was an error while loading. Please reload this page.