SebastianGruza commented on issue #3090:
URL: https://github.com/apache/hugegraph/issues/3090#issuecomment-5721344424
Back to item 1 (the property codec) with a full trace after #3184 landed on
master (`1a15e762`). Two corrections to my 2026-09-16 comments first, then
measurements and a proposal.
### Corrections
- I wrote that the paged full scan (`has('~page', '')`) reaches
`HstoreTable.queryAll()` as a `ConditionQuery`. Wrong:
`HugeGraphStep.addHasContainer()` (line 184) consumes `~page` via `setPage()`,
the has-container list stays empty and `makeQuery()` builds a plain `Query`, so
`queryAll()` pushes `null`. Measured: `GET
/graph/{edges,vertices}?page=&limit=5` → 200, no decoder errors.
- I wrote that whether `queryAll()` and the shard overload fail "is data
dependent". Also wrong; both are safe for reasons visible in the code: the
shard overload `queryByRange(Session, Shard, ConditionQuery)` calls
`session.scan(table, codeFrom, codeTo, scanType, query)`, and that overload in
`HstoreSessionsImpl` (lines 757-767 and 770-780) discards the condition bytes
and sends `new byte[0]`. Measured: 12-shard scan through
`/traversers/edges/scan` → 106 edges, 0 errors, although every one of them has
a DOUBLE as its first property.
### The codec: every type fails, and schema does not change that
Cross-module test: a value written with the server
`BytesBuffer.writeProperty(PropertyKey, value)` (hugegraph-core), read back
with the struct `BytesBuffer.readProperty(PropertyKey)` (hugegraph-struct),
which is exactly what `FilterIterator.hasNext()` does on the store for every
pushed-down query. Master `1a15e762`, JDK 21:
| type, value | server bytes | struct `readProperty` |
|---|---|---|
| DOUBLE 1.5 | `3ff8000000000000` | `Can't construct Cardinality from code
0` |
| DOUBLE 63.5 | `404fc00000000000` | `Unsupported data type UNKNOWN` |
| LONG 100 | `64` | `Can't construct DataType from code 36` |
| TEXT "ETC" | `03455443` | `Can't construct Cardinality from code 0` |
| INT 7 · BOOLEAN true · FLOAT 0.5 · DATE 0 | `07` · `01` · `3f000000` ·
`00` | `Can't construct Cardinality from code 0` |
No type gets through; which exception fires depends only on the first byte
of the value. Relevant for the fix design: the struct
`readProperty(PropertyKey)` (`BytesBuffer.java:587-596`) *always* reads the
header byte and only then sets `cardinality`/`dataType` on the key it was
given, so the schema-aware path (`BusinessHandlerImpl` around line 700,
`getGraphSupplier(graph)`) uses the schema for the key's identity only, not for
decoding. Wiring a `HugeGraphSupplier` into `FilterIterator` fixes nothing on
its own. Test source and output:
https://github.com/SebastianGruza/hugegraph-validation/tree/master/results/issue-3090/codec.
The other side of the channel works: the server `ConditionQuery.bytes()`
round-trips through the struct `ConditionQuery.fromBytes()` with
`resultType=EDGE` and the conditions intact (`[LABEL == 1, 1 > 1.0]`), so if
the bytes reached the store, parsing would start and fail on the first row with
properties.
### Why master does not fail: the server no longer sends any condition that
forces a parse
I traced every place where `HstoreTable` serializes a `ConditionQuery` and
measured each on the lab (master `1a15e762`, 1 PD + 3 stores, edges with a
DOUBLE as the first property; 0 decoder errors in the server and store logs
after the whole series):
| path | what reaches the store | why | measured |
|---|---|---|---|
| `queryByPrefix` / `queryByRange(IdRangeQuery)` by sort key, with an extra
non-sort-key property (`has('asset','ETC').has('epoch', gt(50)).has('amount',
gt(1.0))`) | nothing | `GraphTransaction.optimizeQuery()` (lines 1600-1614)
does `query.copy()` + `resetUserpropConditions()` once the sort keys match;
`HstoreStore.query()` (line 400) copies the already optimized query as
`originQuery`, so `prepareConditionQuery()` sees sysprops only and returns
`null` after #3184; the rest is filtered by the server | 7 REST adjacency
filters, including equality and range on `amount`: 200, correct sets |
| non-sort-key property without an index (`g.E().has('amount', gt(1.0))`,
`g.V().hasLabel('person').has('name','zzz')`) | nothing | rejected in
`GraphIndexTransaction`: `NoIndexException` | 7 Gremlin queries: 500
NoIndexException, store untouched |
| `queryAll()`, both exits | nothing | only a plain `Query` gets there:
`~page` is consumed in `HugeGraphStep`, label queries go to the label index or
`NoIndexException` (`supportsQueryByLabel()==false`) | paged edge and vertex
scans: 200 |
| `queryByRange(Session, Shard, ConditionQuery)` (`/traversers/*/scan`) |
empty array | `HstoreSessionsImpl.scan(table, codeFrom, codeTo, …)` drops
`query` | 12 shards, 106 edges, 0 errors |
| sysprop-only pushdown from `queryByRange` | nothing since `1a15e762` |
#3184 | 13/13 sort-key shapes, id sets equal to rocksdb (in the PR) |
Conclusion: after #3184 there is no OLTP query that delivers
`ConditionQuery` bytes to `FilterIterator`. The whole store-side "operator
sinking" (`FilterIterator`, sort/aggregation through `SortShuffleSerializer`
and `AggregativeQueryObserver`, `indexIntersection`) is dead code for
server-written data, because each of those paths starts by parsing the row with
the same `readProperty`. #3184 did not fix item 1; it cut the last path by
which the server reached the broken parser. That changes the problem statement:
not "some queries fail" but "the feature is disabled by an incompatible codec,
and two guards keep it away from traffic".
### Proposed direction (simpler than the versioned blob from 2026-09-02)
On 2026-09-02 I proposed a versioned pushdown blob carrying schema hints.
After the trace that is unnecessary: the store has the schema. The struct
`SchemaDriver` reads
`HUGEGRAPH/{cluster}/GRAPHSPACE/{gs}/GRAPH/{graph}/SCHEMA/PROPERTY_KEY/…` from
PD, the same path the server `SchemaMetaManager` writes, and
`BusinessHandlerImpl.getGraphSupplier(graph)` already builds a `SchemaGraph`
from it. So the codec can simply be unified on the server's schema-driven
format:
1. struct `BytesBuffer.readProperty/writeProperty(PropertyKey)`: decode by
the key's `dataType`/`cardinality`, no header byte, identical to hugegraph-core;
2. `BinaryElementSerializer.parseProperty()` requires a `HugeGraphSupplier`;
`FilterIterator.of()` and `HgStoreWrapperEx` pass `getGraphSupplier(graph)`
instead of `null` (edge label from the schema as well, instead of `UNDEF`);
3. the store's internal writers
(`SortShuffleSerializer.writeVertex/writeEdge`, `AggregativeQueryObserver`)
move to the same codec. No data migration: the store has no persisted rows in
its own format, spill files and aggregation responses are transient;
4. align the `prepareConditionQuery()` / `prepareConditionQueryList()`
guards (item 2 of my 2026-09-16 list);
5. a cross-module test in CI: server writes, struct reads, all types and
cardinalities, plus a `Comparable` test for `Condition.RelationType.compare` on
the struct side;
6. only afterwards, separately: decide whether the server should push
user-property conditions again (today it pushes none), i.e. whether sinking
returns to traffic.
Rolling upgrades are not a concern, because no bytes reach the store today:
an old server with a new store and a new server with an old store behave
exactly as now. Consequence for item 6: without it, 1-5 is a latent fix visible
only in tests, and I treat it as such.
@imbajin @bitflicker64: is this the direction you want? If so, I'll take 1-5
as one PR across struct + store + hstore, with the test from item 5 as the
first commit (red on master). If you would rather keep sinking disabled and
remove the dead code, I can prepare that too, but I'd prefer not to guess.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
