aminghadersohi commented on PR #44739:
URL: https://github.com/apache/superset/pull/44739#issuecomment-5905704774
@fitzee thanks for the thorough review. All four points are addressed;
master had not moved, so there's no new merge.
1. **Sort performance (8d332399ab).** Your number-vs-number fast path is at
the top of `sortResults`, and each value's parsed sort key is memoized in a
bounded cache that clears above 250k entries. Node 22, 100k rows, median of 5:
| Column | master | fe32227 | 8d332399ab |
|---|---|---|---|
| plain numbers | 30 ms | 925 ms | 32 ms |
| decimal strings | 500 ms | 1294 ms | 226 ms |
| text | 102 ms | 467 ms | 140 ms |
New tests cover direct number comparisons (`-0`/`0`, infinities, `0.1 +
0.2` vs `0.3`). They also check that a number and its exact string, such as
`1e21` and `'1E+21'`, compare equal, so the fast path agrees with the parsed
path.
2. **Transitivity (8d332399ab).** The comparator orders by type first:
numbers (exact decimal strings and ±Infinity included), then text, then `NaN`,
then nulls. It then compares within each type. `[9,'5x',10]`, `[10,9,'5x']`,
`['5x',10,9]` and their string versions all sort to `9, 10, '5x'`. A
brute-force test checks reflexivity, antisymmetry and transitivity over every
triple of 23 mixed values. The misleading comment is gone, and so is
`isNumericText`, which only served the old text fallback. Six of the new tests
fail against the previous comparator. FilterableTable Jest: 67 pass locally.
3. **Scientific notation fallback (0241530286).** `stringify_values` uses
`format(v, "f")` for finite Decimals and `None` for `NaN`/`Infinity`. Tests
cover `[Decimal('Infinity'), Decimal('-0.00000010')]` → `[None,
'-0.00000010']`, Decimal `NaN`/`-Infinity`/`1E+3`, and Decimal mixed with float
(`-0.00000010`, `1.5`, `0E-18`). Each test also asserts the CSV has no
apostrophe escape and no `E±` notation. All three fail before the change. This
keeps the "never in scientific notation" line in the docs accurate.
4. **Extension API (1b68a7bcdb).** The UPDATING entry says the `data` rows
given to `sqlLab.onDidQuerySuccess` listeners carry strings for `numeric`
columns, and that `+` concatenates them. The API is named `onDidQuerySuccess`
in `@apache-superset/core`. The entry also notes fixed-point notation and
`null` for non-finite decimals.
The PR description is updated with the new behaviour and the benchmark.
--
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]
