feat(index): add vector index option surface and validation - #19310
feat(index): add vector index option surface and validation#19310chrevanthreddy wants to merge 1 commit into
Conversation
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR adds the option/config surface and validation for vector indexes (VectorIndexOptions, VectorIndexType, VectorDistanceMetric) plus two MDT partition-name constants, with nothing wired into runtime paths yet. A couple of things worth double-checking in the inline comments — mainly locale-sensitive case parsing and whether the required dimension value should be range-validated. Please take a look at any inline comments, and this should be ready for a Hudi committer or PMC member to take it from here. Two small naming nits worth a quick look.
| * @return matching enum constant | ||
| */ | ||
| public static VectorDistanceMetric fromString(String name) { | ||
| return valueOf(name.toUpperCase().replace(" ", "_").replace("-", "_")); |
There was a problem hiding this comment.
🤖 name.toUpperCase() uses the default locale here. In a Turkish-locale JVM "cosine"/"ivfflat" uppercase to COSİNE/İVFFLAT (dotted İ), so valueOf(...) throws on otherwise-valid input. Elsewhere in hudi-common this is pinned to Locale.ROOT (e.g. WriteConcurrencyMode, WriteOperationType). Same pattern applies to VectorIndexType.fromString (line 57) and the toUpperCase()/toLowerCase() calls in VectorIndexOptions — getQuantizer (293), getRaBitQStorage (346), getSearchMode (376). Could you pin these to Locale.ROOT?
There was a problem hiding this comment.
Fixed by using Locale.ROOT for metric and algorithm parsing; quantizer normalization now uses Locale.ROOT as well.
| throw new IllegalArgumentException( | ||
| "Vector index requires '" + DIMENSION + "' in OPTIONS"); | ||
| } | ||
| return Integer.parseInt(v); |
There was a problem hiding this comment.
🤖 Since this PR is the validation layer, would it be worth also checking dimension > 0 here? Right now 'vector.dimension' = '0' or a negative value parses cleanly and only surfaces much later during index build (e.g. a NegativeArraySizeException when allocating float[dimension]), far from the config site. getRaBitQBits already does an early range check, so a matching guard here would keep the fail-early story consistent.
There was a problem hiding this comment.
Added an explicit dimension > 0 validation and focused coverage for zero and negative dimensions.
| */ | ||
| public enum VectorDistanceMetric implements Serializable { | ||
|
|
||
| /** |
There was a problem hiding this comment.
🤖 nit: all Java enums already implement java.io.Serializable by contract, so implements Serializable here is redundant — could you drop it (and the import) to avoid implying there's something special about the serialization of this enum?
There was a problem hiding this comment.
Removed the redundant Serializable declaration and import.
| public static final int DEFAULT_REFINE_FACTOR = 10; | ||
| public static final int DEFAULT_MAX_ITER = 20; | ||
| public static final long DEFAULT_RABITQ_SEED = 42L; | ||
| public static final int DEFAULT_RABITQ_BITS = 2; |
There was a problem hiding this comment.
🤖 nit: all other defaults follow the DEFAULT_<KEY_NAME> pattern (e.g. NUM_CLUSTERS → DEFAULT_NUM_CLUSTERS), but here the key is RABITQ_RANDOM_SEED while the default is DEFAULT_RABITQ_SEED (drops _RANDOM). Could you rename this to DEFAULT_RABITQ_RANDOM_SEED for consistency?
There was a problem hiding this comment.
Renamed the default to DEFAULT_RABITQ_RANDOM_SEED and updated all uses.
de7e166 to
cda1749
Compare
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR adds the self-contained option/config surface and early validation for CREATE INDEX ... USING VECTOR (RFC-109), plus two new MDT partition-name constants — no runtime wiring yet. The logic is straightforward; a couple of consistency points around locale-safe normalization and validation of the string-typed options are worth double-checking in the inline comments. Please take a look at any inline comments, and this should be ready for a Hudi committer or PMC member to take it from here. A few naming-consistency nits in the boolean config-reader helpers.
| } | ||
|
|
||
| public static String getRaBitQStorage(Map<String, String> opts) { | ||
| return opts.getOrDefault(RABITQ_STORAGE, DEFAULT_RABITQ_STORAGE).toLowerCase(); |
There was a problem hiding this comment.
🤖 This .toLowerCase() (and the one in getSearchMode below) still uses the default locale — the earlier metric/algorithm/quantizer fixes switched to Locale.ROOT for exactly this reason. On a Turkish-locale JVM an input like HIDDEN_COLUMNS or APPROXIMATE lowercases to hıdden_columns/approxımate (dotless ı) and silently misses the equality check. Could you use toLowerCase(Locale.ROOT) here too?
| } | ||
|
|
||
| public static String getSearchMode(Map<String, String> opts) { | ||
| return opts.getOrDefault(SEARCH_MODE, DEFAULT_SEARCH_MODE).toLowerCase(); |
There was a problem hiding this comment.
🤖 Since this PR is specifically the validation layer, I wonder if the string-typed enum-like options (quantizer, storage, search mode, offset-index policy) should also fail fast on unknown values the way metric/algorithm do via valueOf. Right now 'vector.search.mode' = 'aproximate' silently falls back to exact, and 'vector.rabitq.storage' = 'hiden_columns' silently disables MDT storage — both easy to miss. Worth validating against the allowed set here?
| return "approximate".equals(getSearchMode(opts)); | ||
| } | ||
|
|
||
| public static boolean isDataTableClusterByIvf(Map<String, String> opts) { |
There was a problem hiding this comment.
🤖 nit: this class already uses should… for boolean config readers (shouldMaterializeRaBitQColumnsOnCreate, shouldStoreRaBitQCodesInMdt) — could you rename isDataTableClusterByIvf, isRaBitQResidualEncoding, and isRaBitQAsymmetricScoring to shouldClusterDataTableByIvf, shouldUseRaBitQResidualEncoding, and shouldUseRaBitQAsymmetricScoring to make the convention consistent throughout the class?
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #19310 +/- ##
============================================
+ Coverage 70.49% 72.11% +1.62%
+ Complexity 32775 32549 -226
============================================
Files 2700 2564 -136
Lines 152896 148389 -4507
Branches 19009 18659 -350
============================================
- Hits 107778 107012 -766
+ Misses 36596 32914 -3682
+ Partials 8522 8463 -59
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
4 participants
Describe the issue this Pull Request addresses
Resolves #19096 (umbrella: #19094, RFC-109)
First foundational slice of RFC-109 (Hudi Native Vector Index): the option surface and validation for vector indexes. This PR is intentionally scoped to the self-contained option/config layer so it compiles and tests standalone; the DDL command dispatch, MDT schema/payload, and bootstrap execution land in dependent follow-up PRs (#19097, #19099, …) where their partition-type and executor dependencies are introduced.
Summary and Changelog
org.apache.hudi.common.index.vector.VectorIndexOptions— the fullOPTIONS(...)key surface forCREATE INDEX ... USING VECTOR(dimension, metric, algorithm, quantizer, cluster/probe/refine counts, RaBitQ bits/seed/storage/posting layout, search-time flags) with typed accessors, defaults, and early validation.VectorIndexType— routing algorithm enum (IVFFLAT,IVFPQ_HNSW) with case/separator-insensitive parsing.VectorDistanceMetric—COSINE/L2/DOT_PRODUCTdistance enum (min-heap-consistent distances) with parsing.PARTITION_NAME_VECTOR_INDEX/PARTITION_NAME_VECTOR_INDEX_PREFIXconstants toHoodieTableMetadataUtil.TestVectorIndexOptions— 9 unit tests covering defaults, metric/algorithm parsing, and early-failure on invalid configs (missing dimension, invalid metric, out-of-range RaBitQ bits).Impact
No behavioral impact on existing tables or code paths. Adds new, unreferenced option/enum classes plus two constants. No schema, writer, reader, or DDL execution changes in this PR.
Risk Level
none
New self-contained classes + two constants + unit tests; nothing wires into existing runtime paths yet.
Documentation Update
Option semantics are documented as Javadoc on each option key. The user-facing RFC document is tracked separately (see #19309). No site docs change required until the feature is user-invokable.
Contributor's checklist