aminghadersohi commented on code in PR #44739: URL: https://github.com/apache/superset/pull/44739#discussion_r4143459166
########## superset-frontend/src/components/FilterableTable/sortResults.ts: ########## @@ -0,0 +1,156 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +type CellValue = string | number | null; + +// Every pattern below is a single flat run of digits, so matching stays linear +// in the input length; the optional parts are split off by hand instead of +// nesting quantified groups, which could backtrack catastrophically. +const DIGITS = /^\d*$/; +const EXPONENT = /^[+-]?\d+$/; + +/** Split `text` at its first `e`/`E`; `null` when the exponent is malformed. */ +function splitExponent(text: string): [string, string | undefined] | null { + const at = text.search(/[eE]/); + if (at === -1) return [text, undefined]; + const exponent = text.slice(at + 1); + return EXPONENT.test(exponent) ? [text.slice(0, at), exponent] : null; +} + +// Compare significands and decimal orders, never coerce exact strings to Number. +// Keeping the exponent separate also avoids allocating 10**exponent zeroes. +export function decimalParts(value: Exclude<CellValue, null>) { + const text = String(value); + const signed = text[0] === '+' || text[0] === '-'; + const parts = splitExponent(signed ? text.slice(1) : text); + if (!parts) return null; + const [mantissa, exponent] = parts; + const dot = mantissa.indexOf('.'); + const integer = dot === -1 ? mantissa : mantissa.slice(0, dot); + const fraction = dot === -1 ? '' : mantissa.slice(dot + 1); + if ( + !(integer || fraction) || + !DIGITS.test(integer) || + !DIGITS.test(fraction) + ) { + return null; + } + const digits = `${integer}${fraction}`.replace(/^0+/, ''); + return { + sign: digits ? (text[0] === '-' ? -1 : 1) : 0, + digits, + order: + BigInt(digits.length) + BigInt(exponent ?? '0') - BigInt(fraction.length), + }; +} + +// Values sort by type first, so mixed columns stay transitive: numbers (exact +// decimal strings and infinities included) before text, and NaN after both. +const NUMERIC_RANK = 0; +const TEXT_RANK = 1; +const NAN_RANK = 2; + +type SortKey = + | { + rank: typeof NUMERIC_RANK; + // -1 or 1 for an infinity, 0 for a finite value. + infinity: number; + sign: number; + digits: string; + order: bigint; + } + | { rank: typeof TEXT_RANK; text: string } + | { rank: typeof NAN_RANK }; + +function toSortKey(value: Exclude<CellValue, null>): SortKey { + if (typeof value === 'number' ? Number.isNaN(value) : value === 'NaN') { + return { rank: NAN_RANK }; + } + if (value === Infinity || value === 'Infinity') { + return { + rank: NUMERIC_RANK, + infinity: 1, + sign: 1, + digits: '', + order: BigInt(0), + }; + } + if (value === -Infinity || value === '-Infinity') { + return { + rank: NUMERIC_RANK, + infinity: -1, + sign: -1, + digits: '', + order: BigInt(0), + }; + } + const parts = decimalParts(value); + return parts + ? { rank: NUMERIC_RANK, infinity: 0, ...parts } + : { rank: TEXT_RANK, text: String(value) }; +} + +// Parsing dominates the cost of sorting decimal strings, and a sort compares +// each cell O(log n) times, so keys are cached per value. The cache is cleared +// whenever it outgrows a large result set to keep memory bounded. +const SORT_KEY_CACHE_LIMIT = 250_000; +const sortKeyCache = new Map<Exclude<CellValue, null>, SortKey>(); Review Comment: Fixed in c6915268e046db67588a28ba6a14a9142ba77164. The cache is now a WeakMap keyed by the rows array, and FilterableTable passes its data into the comparator. Cached cell values can be collected along with that result set; standalone comparisons retain no cache. The plain-number fast path and type-ranked comparator are unchanged. Added a regression test for reuse within one result set and isolation across result sets. All 68 FilterableTable tests and pre-commit checks pass. ########## superset/result_set.py: ########## @@ -88,6 +89,11 @@ def stringify_values(array: NDArray[Any]) -> NDArray[Any]: # Non-JSON-serializable value (e.g. bytes, custom # objects): fall back to str() to avoid crashing. obj[...] = str(val) + elif isinstance(val, Decimal): + # str() switches to scientific notation for small or + # large exponents (-1.0E-7), which CSV export would + # formula-escape. NaN and Infinity have no exact value. + obj[...] = format(val, "f") if val.is_finite() else None Review Comment: Agreed: Database.get_df -> load_into_dataframe uses the same result-set fallback. Addressed in d161f307136c0ea28bd5b2459c9aba609079b548 by correcting UPDATING.md and the number-formatting guide: Decimal columns falling back to strings (including mixed Decimal/float columns) use fixed-point notation, and non-finite Decimals become null. Other chart serialization/formatting is unchanged. Added parameterized tests through Database.load_into_dataframe for both cases, without changing the shared behavior. All 26 result-set unit tests and pre-commit checks pass. ########## superset-frontend/src/components/FilterableTable/sortResults.test.ts: ########## @@ -0,0 +1,217 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { decimalParts, sortResults } from './sortResults'; + +test.each([ + ['2', '10', -1], + [ + '12345678901234567890.123456789012345678', + '12345678901234567890.123456789012345679', + -1, + ], + [ + '-12345678901234567890.123456789012345678', + '-12345678901234567890.123456789012345679', + 1, + ], + ['1E-18', '0.000000000000000002', -1], + ['1E+100000', '9E+99999', 1], + ['1E-100000', '9E-100001', 1], + ['0.00000000000000000001', '0', 1], + ['0010.500', 10.5, 0], + ['-0.00', 0, 0], + ['.5', '0.50', 0], + ['9.99', 10, -1], + [null, '1.23', 1], + ['1.23', null, -1], + [null, null, 0], + ['apple', 'pear', -1], + ['2024-01-01', '2025-01-01', -1], + ['-Infinity', '-1.2', -1], + ['Infinity', '1.2', 1], +] as const)('sorts %s vs %s exactly', (a, b, expected) => { + expect(sortResults(a, b)).toBe(expected); +}); + +test.each([ + ['apple', 5], + ['apple', 'NaN'], + [NaN, 1.5], + ['2024-01-01', 7], +] as const)('orders %s against %s in one direction only', (a, b) => { + expect(sortResults(a, b)).toBe(-sortResults(b, a)); +}); + +test('treats two NaNs as equal rather than unordered', () => { + expect(sortResults(NaN, NaN)).toBe(0); + expect(sortResults('NaN', 'NaN')).toBe(0); +}); + +test('sorts a mixed text and number column transitively', () => { + const column = [5, 'apple', 'NaN', 10, 'banana']; + const ascending = [...column].sort(sortResults); + const descending = [...column].reverse().sort(sortResults); + expect(descending).toEqual(ascending); +}); + +test.each([ + [[9, '5x', 10]], + [[10, 9, '5x']], + [['5x', 10, 9]], + [['9', '5x', '10']], + [['10', '9', '5x']], + [['5x', '10', '9']], +])('sorts numbers before text whatever the input order: %p', column => { + const sorted = [...column].sort(sortResults).map(String); + expect(sorted).toEqual(['9', '10', '5x']); +}); Review Comment: Verified at d161f307136c0ea28bd5b2459c9aba609079b548; this finding is not applicable, so no test change is needed. Each test.each row is already nested as [[9, '5x', 10]] (and similarly for the other five rows). Jest spreads the outer row, passing the entire inner array as column, not the number 9. All six cases pass in the full FilterableTable run (68 tests passed). -- 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]
