bito-code-review[bot] commented on code in PR #44739:
URL: https://github.com/apache/superset/pull/44739#discussion_r4129984152
##########
tests/unit_tests/dataframe_test.py:
##########
@@ -363,3 +365,44 @@ def
test_df_to_records_with_json_serialization_like_sql_lab() -> None:
)
parsed_no_flag = superset_json.loads(json_str_no_flag)
assert parsed_no_flag == parsed # Same result
+
+
[email protected](
+ "value",
+ [
+ "12345678901234567890.123456789012345678",
+ "-12345678901234567890.123456789012345678",
+ "0.000000000000000001",
+ "10.50",
+ "0.00",
+ "1E+30",
+ ],
+)
+def test_decimal_records_keep_all_digits(value: str) -> None:
Review Comment:
<!-- Bito Reply -->
The addition of docstrings to the new test functions is a positive step that
improves code clarity and maintainability. While other existing tests in the
file may lack them, providing docstrings for new contributions aligns with best
practices and ensures the intent of the tests is immediately clear to other
developers.
**tests/unit_tests/dataframe_test.py**
```
@pytest.mark.parametrize(
"value",
[
"12345678901234567890.123456789012345678",
"-12345678901234567890.123456789012345678",
"0.000000000000000001",
"10.50",
"0.00",
"1E+30",
],
)
def test_decimal_records_keep_all_digits(value: str) -> None:
"""Test that decimal values are preserved as strings without precision
loss."""
```
##########
tests/unit_tests/dataframe_test.py:
##########
@@ -363,3 +365,44 @@ def
test_df_to_records_with_json_serialization_like_sql_lab() -> None:
)
parsed_no_flag = superset_json.loads(json_str_no_flag)
assert parsed_no_flag == parsed # Same result
+
+
[email protected](
+ "value",
+ [
+ "12345678901234567890.123456789012345678",
+ "-12345678901234567890.123456789012345678",
+ "0.000000000000000001",
+ "10.50",
+ "0.00",
+ "1E+30",
+ ],
+)
+def test_decimal_records_keep_all_digits(value: str) -> None:
+ decimal = Decimal(value)
Review Comment:
<!-- Bito Reply -->
The suggestion to rename the variable is not strictly necessary, as the
local variable does not shadow the module name in this context. The code is
clear and correct as written, and you may choose to ignore the suggestion.
##########
superset-frontend/src/components/FilterableTable/sortResults.ts:
##########
@@ -0,0 +1,64 @@
+/**
+ * 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;
+
+// Compare significands and decimal orders, never coerce exact strings to
Number.
+// Keeping the exponent separate also avoids allocating 10**exponent zeroes.
+const DECIMAL = /^([+-]?)(?:(\d+)(?:\.(\d*))?|\.(\d+))(?:[eE]([+-]?\d+))?$/;
+
+function decimalParts(value: Exclude<CellValue, null>) {
+ const match = DECIMAL.exec(String(value));
+ if (!match) return null;
+ const fraction = match[3] ?? match[4] ?? '';
+ const digits = `${match[2] ?? ''}${fraction}`.replace(/^0+/, '');
+ return {
+ sign: digits ? (match[1] === '-' ? -1 : 1) : 0,
+ digits,
+ order:
+ BigInt(digits.length) + BigInt(match[5] ?? '0') -
BigInt(fraction.length),
+ };
+}
+
+export function sortResults(valueA: CellValue, valueB: CellValue): number {
+ if (valueA === valueB) return 0;
+ if (valueA === null) return 1;
+ if (valueB === null) return -1;
+
+ const a = decimalParts(valueA);
+ const b = decimalParts(valueB);
+ if (a && b) {
+ if (a.sign !== b.sign) return a.sign < b.sign ? -1 : 1;
+ if (a.sign === 0) return 0;
+ if (a.order !== b.order) return (a.order < b.order ? -1 : 1) * a.sign;
+ const width = Math.max(a.digits.length, b.digits.length);
+ const left = a.digits.padEnd(width, '0');
+ const right = b.digits.padEnd(width, '0');
+ return left === right ? 0 : (left < right ? -1 : 1) * a.sign;
+ }
+
+ // Retain the table's existing numeric-string, text and infinity behavior.
+ const numberOrText = (value: Exclude<CellValue, null>) =>
+ typeof value === 'string' &&
+ /^(NaN|-?((\d*\.\d+|\d+)([Ee][+-]?\d+)?|Infinity))$/.test(value)
+ ? Number(value)
+ : value;
+ const left = numberOrText(valueA);
+ const right = numberOrText(valueB);
+ return left === right ? 0 : left < right ? -1 : 1;
Review Comment:
<!-- Bito Reply -->
The verification that the fix remains unchanged and that the NaN and
mixed-type test cases continue to pass is noted. This confirms that the logic
implemented to handle comparator contract violations remains effective and
stable at the current head.
--
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]
