deepakpanda93 commented on code in PR #19650:
URL: https://github.com/apache/hudi/pull/19650#discussion_r3796481153
##########
website/learn/tech-specs.md:
##########
@@ -532,12 +596,32 @@ For example, a secondary index on the `city` column, for
a record with `city = C
chennai$id1 -> {"isDeleted": false}
```
-Each secondary-index partition is tagged with a `HoodieIndexVersion` (stored
on the corresponding `HoodieIndexDefinition`).
-Table version 8 constrained every secondary-index partition to `V1` (the
encoding described above). Table version 9
-introduces `V2`, which shards records by the primary (record) key rather than
by the secondary key. This makes secondary-index
-updates cheaper on writes with skewed secondary values, at the cost of
secondary-key range scans having to visit more file
-groups. Readers pick their scan strategy from the per-partition
`HoodieIndexVersion`. New tables created on version 9
-default to `V2` for secondary indexes; existing `V1` partitions from upgraded
tables continue to be read with the V1 encoding.
+**Partitioning** decides which file group of the index partition an entry is
written to. Hudi hashes a portion of the
+record key and takes that value modulo the number of file groups. Which
portion is hashed is governed by the
+[`HoodieIndexVersion`](#index-versions) recorded on the index's
`HoodieIndexDefinition`:
+
+* **`V1`**, the default for table version 8, hashes the whole
`<escaped-secondary-key>$<escaped-primary-key>` key.
+ Entries sharing a secondary value are distributed across all file groups,
so resolving a secondary value to its
+ records reads every file group unless the primary key is already known.
+* **`V2`**, the default for table version 9, hashes only the leading
`<escaped-secondary-key>$` portion. All entries
+ sharing a secondary value therefore reside in one file group, and a lookup
by secondary value alone reads that
+ single file group.
+
+The strategy is selected per partition from its recorded version, so `V1`
partitions on an upgraded table continue to be
+read as `V1` while indexes created afterwards on the same table use `V2`.
+
+#### Limitations
+
+* A secondary index may be defined on **exactly one column**. Attempting
more fails with
+ `Only one column can be indexed for functional or secondary index.`
+* The indexed column must be one of `string`, `int`, `long`, `float`,
`double`, `date`, `time`, or a
Review Comment:
Good catch, and you are right. Fixed in da0d2013.
I want to record why the original wording went wrong, because the mechanism
is the interesting part and it is worth having in the thread.
`HoodieIndexUtils#isSecondaryIndexSupportedType` is an allow-list over
*schema* types, and it has no `SHORT` or `BYTE` cases:
```java
switch (schema.getType()) {
case STRING: case INT: case LONG: case DOUBLE: case FLOAT: case DATE: case
TIME:
return true;
case TIMESTAMP:
return ((HoodieSchema.Timestamp) schema).isUtcAdjusted();
default:
return false;
}
```
I read that literally and wrote `int`, which is accurate about the schema
type and misleading as SQL. Avro has neither a short nor a byte type, and
Spark's `ShortType` and `ByteType` serialize to Avro `INT` — `AvroSerializer`
carries explicit `case (ByteType, INT)` and `case (ShortType, INT)` branches.
So a `smallint` or `tinyint` column arrives at the `INT` branch of the
allow-list and is accepted. The allow-list never needed to name them.
`TestSecondaryIndexDataTypes` confirms it from the other direction, and its
two sets are more complete than what I had written:
- succeeds: `col_string`, `col_int`, `col_bigint`, `col_long`,
`col_smallint`, `col_tinyint`, `col_timestamp`, `col_date`, `col_float`,
`col_double`
- fails: `col_decimal`, `col_boolean`, `col_binary`, `col_array`, `col_map`,
`col_struct`
The bullet now reads:
> The indexed column must resolve to one of the schema types `string`,
`int`, `long`, `float`, `double`, `date`, `time`, or a **UTC-adjusted**
`timestamp`. In Spark SQL terms that covers `string`, `tinyint`, `smallint`,
`int`, `bigint`, `float`, `double`, `date` and `timestamp`, since `tinyint` and
`smallint` are represented as `int`. Rejected are `decimal`, `boolean`,
`binary`, the nested types `array`, `map` and `struct`, and local
(non-UTC-adjusted) timestamps. A nullable column is supported when its non-null
branch is a supported type.
That gives both vocabularies rather than picking one, and it names the
rejected types explicitly. The previous text disposed of them as "all other
types", which is technically true but of no help to someone wondering whether
their `decimal` column can be indexed.
I kept `time` in the schema-type list even though no Spark SQL type maps to
it today, since it is in the allow-list and this page documents the format
rather than one engine's bindings.
The UTC-adjusted caveat survives this change and is worth keeping in view:
`col_timestamp` passes in that test because Spark's `TIMESTAMP` maps to a
UTC-adjusted timestamp, whereas `TIMESTAMP_NTZ` is local and would be rejected.
Build passes with the warning block still byte-identical to a baseline built
at the same base commit, and the rendered page shows all of `tinyint`,
`smallint`, `bigint`, `decimal`, `boolean`, `binary` and `struct`.
On the rest of the review: thanks for verifying the index-version table, the
table-version-9 validation rules, the `column_stats` to `partition_stats` drop
on downgrade, the V1-vs-V2 correction and the quoted error strings against
source. Agreed that a committer should confirm before merge — @yihua and
@nsivabalan, the upgrade/downgrade section is the part I would most like a
second pair of eyes on, since it is verified by reading
`EightToNineUpgradeHandler` and `UpgradeDowngradeUtils` rather than by running
an actual upgrade and downgrade against a 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]
