aminghadersohi commented on code in PR #44739:
URL: https://github.com/apache/superset/pull/44739#discussion_r4144406647
##########
superset/dataframe.py:
##########
@@ -85,8 +85,27 @@ def _is_trusted_missing_or_nonfinite(value: Any) -> bool:
return False
+def _convert_decimals(value: Any) -> Any:
+ """Keep SQL Lab decimals exact across JSON, MessagePack and browser
parsing."""
+ # Match exact types only, as below, so object-column values never get to
run
+ # overridden ``items``/``__iter__`` hooks during conversion.
+ value_type = type(value)
+ if value_type is Decimal:
+ return format(value, "f") if Decimal.is_finite(value) else None
+ if value_type is dict:
+ return {key: _convert_decimals(item) for key, item in
dict.items(value)}
+ if value_type is list:
+ return [_convert_decimals(item) for item in value]
+ if value_type is tuple:
+ return tuple(_convert_decimals(item) for item in value)
+ return value
+
+
def df_to_records(
- dframe: pd.DataFrame, *, convert_big_integers: bool = True
+ dframe: pd.DataFrame,
+ *,
+ convert_big_integers: bool = True,
+ convert_decimals: bool = True,
Review Comment:
Fixed in 48bf663e610f888077751622804b7e72bfd14737: decimal conversion
defaults to False, all three SQL Lab callers explicitly opt in, and the chart
opt-out is removed. Default-call coverage verifies numeric JSON, while the
existing sync and async SQL Lab assertions still verify exact strings.
##########
UPDATING.md:
##########
@@ -89,6 +89,23 @@ cleartext.
Connections using the separate MariaDB engine (mariadb:// URIs) and other
MySQL-compatible engines keep their existing SSL handling.
+### SQL Lab decimal results use exact strings
+
+SQL Lab represents database `DECIMAL`/`NUMERIC` values as JSON strings instead
+of JSON numbers, preserving precision and trailing zeros in the results grid
+and exports. The strings use fixed-point notation, and non-finite `NaN` and
+`Infinity` decimals are returned as `null`. Numeric sorting still compares
+their exact values. API consumers performing arithmetic should parse these
+strings with a decimal library, not JavaScript `Number`. This includes SQL Lab
+extensions: the `data` rows passed to `sqlLab.onDidQuerySuccess` listeners
Review Comment:
Fixed in 48bf663e610f888077751622804b7e72bfd14737: UPDATING.md and the
number-formatting guide name the sqleditor.extension.resultTable override and
explain that it receives decimal strings without the built-in comparator, so
extensions must implement decimal sorting.
##########
superset-frontend/src/components/FilterableTable/sortResults.test.ts:
##########
@@ -0,0 +1,237 @@
+/**
+ * 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']);
+});
+
+test('orders numbers, then text, then NaN, then nulls', () => {
+ const column = [
+ 'NaN',
+ null,
+ 'apple',
+ 3,
+ NaN,
+ '2.5',
+ 'Infinity',
+ '-Infinity',
+ -Infinity,
+ '10',
+ ];
+ expect([...column].sort(sortResults)).toEqual([
+ '-Infinity',
+ -Infinity,
+ '2.5',
+ 3,
+ '10',
+ 'Infinity',
+ 'apple',
+ 'NaN',
+ NaN,
+ null,
+ ]);
+});
+
+test('is a consistent total order over mixed values', () => {
+ const values = [
+ null,
+ NaN,
+ 'NaN',
+ -Infinity,
+ '-Infinity',
+ Infinity,
+ 'Infinity',
+ -1.5,
+ '-1.50',
+ 0,
+ -0,
+ '0.000',
+ '1E-18',
+ 0.1,
+ '0.1',
+ 9,
+ '10',
+ 1e21,
+ '1E+21',
+ '',
+ '5x',
+ 'apple',
+ '2024-01-01',
+ ];
+ const sign = (n: number) => Math.sign(n) || 0;
+ values.forEach(a => {
+ expect(sortResults(a, a)).toBe(0);
+ values.forEach(b => {
+ const ab = sign(sortResults(a, b));
+ expect(sign(sortResults(b, a))).toBe(-ab || 0);
+ values.forEach(c => {
+ const bc = sign(sortResults(b, c));
+ if (ab <= 0 && bc <= 0) {
+ expect(sign(sortResults(a, c))).toBeLessThanOrEqual(0);
+ }
+ });
+ });
+ });
+});
+
+test.each([
+ [1, 2, -1],
+ [2, 1, 1],
+ [-0, 0, 0],
+ [-Infinity, -1e308, -1],
+ [Infinity, 1e308, 1],
+ [9007199254740993, 9007199254740992, 0],
+ [0.1 + 0.2, 0.3, 1],
+] as const)('compares plain numbers %p vs %p directly', (a, b, expected) => {
+ expect(sortResults(a, b)).toBe(expected);
+});
+
+test.each([
+ [0.1, '0.1'],
+ [1e21, '1E+21'],
+ [1e-7, '0.0000001'],
+ [-2.5, '-2.50'],
+ [Infinity, 'Infinity'],
+])('orders number %p and its exact string %p equally', (number, text) => {
+ expect(sortResults(number, text)).toBe(0);
+ expect(sortResults(text, number)).toBe(0);
+});
+
+// Reference regex the linear parser must agree with.
+const REFERENCE_DECIMAL =
+ /^([+-]?)(?:(\d+)(?:\.(\d*))?|\.(\d+))(?:[eE]([+-]?\d+))?$/;
+
+function strings(alphabet: string[], maxLength: number): string[] {
+ const all = [''];
+ let level = [''];
+ for (let length = 1; length <= maxLength; length += 1) {
+ level = level.flatMap(prefix => alphabet.map(token => prefix + token));
+ level.forEach(text => all.push(text));
+ }
+ return all;
+}
+
+test('linear parser accepts exactly what the reference regex matches', () => {
+ const alphabet = ['0', '7', '.', 'e', 'E', '+', '-', 'x', 'NaN', 'Infinity'];
+ const mismatches: string[] = [];
+ const inputs = strings(alphabet, 5);
+ expect(inputs).toHaveLength(111111);
+ expect(inputs).toEqual(
+ expect.arrayContaining(['NaN', 'Infinity', '-Infinity']),
+ );
+ inputs.forEach(text => {
+ if ((decimalParts(text) !== null) !== REFERENCE_DECIMAL.test(text)) {
+ mismatches.push(`decimal: ${text}`);
+ }
+ });
+ expect(mismatches).toEqual([]);
+});
+
+test('rejects long adversarial near-miss inputs', () => {
+ const inputs = [
+ `${'1'.repeat(200000)}x`,
+ `.${'1'.repeat(200000)}.`,
+ `1e${'1'.repeat(200000)}e`,
+ `-${'1'.repeat(100000)}.${'1'.repeat(100000)}E+`,
+ ];
+ inputs.forEach(text => {
+ expect(decimalParts(text)).toBeNull();
+ });
+});
+
+test('reuses sort keys only within the same result set', () => {
+ const rows = [{ value: '1.25' }, { value: '2.50' }];
+ const parse = jest.spyOn(global, 'BigInt');
Review Comment:
Fixed in 48bf663e610f888077751622804b7e72bfd14737: replaced the global
BigInt spy with a small exported cache-inspection seam. The test verifies
cached-key identity reuse and isolation between result sets without depending
on parser call counts.
--
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]
