deepakpanda93 commented on code in PR #19650:
URL: https://github.com/apache/hudi/pull/19650#discussion_r3797424971
##########
website/learn/tech-specs.md:
##########
@@ -463,6 +463,70 @@ The other fields can also be optional for writers
depending on whether protectio
### Naming
Indexes are stored under `.hoodie/metadata` storage path, with separate
partitions of the form `<index_type>_<index_name>`.
+### Index Definitions
+
+Every index carries a definition, serialized to JSON under the path in
`hoodie.table.index.defs.path`
+(default `.hoodie/.index_defs/index.json`). All definitions for a table live
in a single file, keyed by the
+metadata-table partition name:
+
+```json
+{
+ "indexDefinitions": {
+ "<partition_name>": {
+ "indexName": "<partition_name>",
+ "indexType": "<index_type>",
+ "indexFunction": "<index_function>",
+ "version": "<index_version>",
+ "sourceFields": ["<column_1>", "<column_2>"],
+ "indexOptions": {}
+ }
+ }
+}
+```
+
+* `indexType` is one of `files`, `column_stats`, `partition_stats`,
`bloom_filters`, `record_index`,
+ `secondary_index` or `expr_index`.
+* `indexFunction` is the transform applied to the source column, `identity`
unless the index is an expression index.
Review Comment:
You're right, and the original wording was wrong in both directions. Fixed
in 1ead844b.
The bullet claimed `identity` unless the index is an expression index. But
an expression index created without an explicit function *also* gets
`identity`, so `identity` never distinguished the two, and non-expression
indexes do not reliably carry it either.
I traced all four sites that build a `HoodieIndexDefinition`, and the field
turns out to be populated inconsistently across them. That is worth spelling
out, because it also refines your suggestion slightly:
| Registration path | `indexFunction` ends up as |
|---|---|
| `HoodieTableMetadataUtil#getIndexPartitionsToInit` | set only when the
prefix is `expr_index_`, so a **secondary index registered here is empty** |
| the other builder in `HoodieTableMetadataUtil` | never calls
`withIndexFunction`, so built-in partitions are **empty** |
| `HoodieIndexUtils#getSecondaryOrExpressionIndexDefinition` (SQL `CREATE
INDEX`, both secondary and expression) |
`options.getOrDefault(EXPRESSION_OPTION, IDENTITY_TRANSFORM)`, so a secondary
index created this way is **`identity`** |
| `HoodieSparkIndexClient#createOrUpdateColumnStatsIndexDefinition` |
`withIndexFunction(PARTITION_NAME_COLUMN_STATS)`, so it carries
**`"column_stats"`** |
That last one is the case I would flag against "empty for the built-in index
types": `column_stats` registered through the Spark client path is neither
empty nor `identity`, it is the partition name. And a secondary index is not
uniformly `identity` either, since the metadata-config path leaves it empty
while the SQL path sets it.
On the empty case specifically, unset is stored as the empty string rather
than `identity`, because the constructor normalises:
```java
this.indexFunction = nonEmpty(indexFunction) ? indexFunction : EMPTY_STRING;
```
The bullet now reads:
> `indexFunction` carries the transform for an expression index, and
defaults to `identity` when an expression index is created without an explicit
function. For every other index type the field is not meaningful, and the value
it ends up with depends on the code path that registered the definition: empty
when the built-in initialisation path registers it, `identity` for a secondary
index created through SQL `CREATE INDEX`, and the partition name for the
column-stats registration path. Use `indexType` to identify an index, not this
field.
I deliberately did not reduce this to a tidy per-type rule, because the code
does not follow one. Describing the field as inconsistent and pointing readers
at `indexType` seemed more useful than a rule a reader could rely on and be
wrong. If a committer would rather the spec assert a single intended contract
here, that is really a question about whether the inconsistency across those
four call sites is intentional or a latent bug worth its own JIRA, and I am
happy to file one.
Build passes with the warning block still byte-identical to a baseline at
the same base commit, and the rendered page shows the new wording with the old
sentence gone.
@yihua @nsivabalan, alongside this, the upgrade/downgrade section remains
the part I would most value a committer's eyes on, since it is verified by
reading `EightToNineUpgradeHandler` and `UpgradeDowngradeUtils` rather than by
running an upgrade and downgrade against a real table.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
