bito-code-review[bot] commented on code in PR #44739:
URL: https://github.com/apache/superset/pull/44739#discussion_r4119322705
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Local shadows stdlib module name</b></div>
<div id="fix">
`decimal` at line 382 shadows the stdlib `decimal` module name and has no
explicit annotation, contrary to BITO.md rule 13153 (explicit local-variable
annotations in test files). Rename to `decimal_value` with an explicit
`Decimal` annotation so the type stays visible and a future `import decimal`
cannot confuse readers.
</div>
</div>
<small><i>Code Review Run #f49a1c</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Missing test docstrings</b></div>
<div id="fix">
New tests `test_decimal_records_keep_all_digits`,
`test_nested_decimal_records`, and
`test_decimal_conversion_does_not_change_other_numbers` (lines 381, 393, 402)
lack docstrings. BITO.md adaptive rule 12147/12148 requires docstrings on all
new test functions so test intent is visible without reading implementation.
Add a one-line docstring per test describing scenario and expected behavior.
</div>
</div>
<small><i>Code Review Run #f49a1c</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-20: Comparator contract violations</b></div>
<div id="fix">
The fallback comparison breaks the comparator contract ag-grid relies on
(`index.tsx:71`). Verified by execution: `sortResults(NaN, NaN)` returns 1
because `Number('NaN')` (line 59) yields NaN and `NaN === NaN`/`NaN < NaN` are
both false; `sortResults('apple', 5)` and `sortResults(5, 'apple')` both return
1, so mixed text/number columns sort non-transitively. Handle NaN explicitly
and compare cross-type values as strings.
([CWE-20](https://cwe.mitre.org/data/definitions/20.html))
</div>
</div>
<small><i>Code Review Run #f49a1c</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
