SebastianGruza commented on PR #3184:
URL: https://github.com/apache/hugegraph/pull/3184#issuecomment-5652968688
@imbajin @contrueCT, a request for a decision on this PR. It is a 12-line
guard in `HstoreTable.queryByRange()`, the same one `queryByPrefix()` has had
for a long time; CI has been green since Sep 7 and @bitflicker64 went through
three rounds without finding a regression ("blocking: no" three times, unit
tests 8/8, checkstyle clean). There has been no reaction since, so I am asking
directly: can it go in ahead of #2994?
The reason I ask about the order: without this guard #2994 cannot be
verified on HStore. Our backend axis for #2994 (the same queries on hstore and
rocksdb, compared as id sets) gave 38 TARGET-ERR failures from #3090 before
#3184 was on the branch, and OK=170 with it. Anyone reviewing #2994 on hstore
will hit the same wall; @contrueCT noted this in #3090 on Sep 3.
### Codecov
The only red check is `codecov/patch`. It is an artifact: the surefire run
of the `hugegraph-hstore` module is not part of the coverage upload, so the
report shows 0 % for the changed lines even though `HstoreTableTest` (8 tests)
covers them; @bitflicker64's review ran that module locally. I have added this
sentence to the PR description so nobody has to work it out.
### What this bug blocks on 1.7.0 with HStore
The reproducer in the description has 3 edges. Below is the same failure on
5 M edges, on a schema with four sort keys (`xfer[btc_addr, seq, height,
asset]`, vertex ids are BTC/ETH-style addresses), 13 query shapes from one
vertex, master `36811483` vs PR head `0ecc10a3`, hstore vs rocksdb on the same
data. The servers were swapped in place without reloading the data.
| shape, from `g.V(v)` | master hstore | master rocksdb | #3184 hstore | id
sets #3184 hstore = rocksdb |
|---|---|---|---|---|
| `outE('xfer')`, no condition | 57 | 57 | 57 | same |
| `.has('btc_addr',A)` prefix 1 | **500 `Can't construct Cardinality from
code 0`** | 2 | 2 | same |
| `.has('btc_addr',A).has('seq',S)` prefix 2 | **500** | 1 | 1 | same |
| prefix 1 + `has('seq',gte(1))` | **500** | 2 | 2 | same |
| prefix 2 + `has('height',between(..))` | **500** | 1 | 1 | same |
| all four keys `eq` | 1 | 1 | 1 | same |
| prefix 3 + `has('asset',gte('a'))` | 1 | 1 | 1 | same |
| `.has('seq',S)` alone (not a prefix, filter) | 1 | 1 | 1 | same |
| `.has('amount',gte(0))` (not a sort key) | 57 | 57 | 57 | same |
| prefix 1 + `has('amount',gte(0))` | **500** | 2 | 2 | same |
| `bothE('xfer').has('btc_addr',A)` | **500** | 2 | 2 | same |
| `...inV().inE('xfer').has('btc_addr',A)` | **500** | 1 | 1 | same |
| prefix 1 + `valueMap()` | **500** | 2 | 2 | same |
On master with HStore **every partial sort-key prefix and every range on a
key other than the last one fails**, including `bothE`, `inE` and reading the
properties; only "no condition", "all keys equal" and non-prefix filters work.
So the whole use case sort keys exist for (the vertex-centric index) is
unavailable on 1.7.0 with HStore. With #3184 all 13 shapes pass and return the
same ids as RocksDB.
Script, raw results and log:
[`results/pr-3184-sortkeys/`](https://github.com/SebastianGruza/hugegraph-validation/tree/master/results/pr-3184-sortkeys).
--
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]
