codeant-ai-for-open-source[bot] commented on PR #36990: URL: https://github.com/apache/superset/pull/36990#issuecomment-3738366057
## Nitpicks 🔍 <table> <tr><td>🔒 <strong>No security issues identified</strong></td></tr> <tr><td>⚡ <strong>Recommended areas for review</strong><br><br> - [ ] <a href='https://github.com/apache/superset/pull/36990/files#diff-284708d992f68953fcc53db62a3973d8182944146110ec8b630aa2694cc37f40R260-R267'><strong>Runtime type assumption</strong></a><br>The new lookup uses (selection || '').toLowerCase() and assumes `selection` is a string. If `isPhysicalColumn(selection)` can be true for objects (e.g., objects with `column_name`), calling `toLowerCase()` directly may throw at runtime. Verify the guard guarantees a string or coerce/extract the column name safely.<br> - [ ] <a href='https://github.com/apache/superset/pull/36990/files#diff-284708d992f68953fcc53db62a3973d8182944146110ec8b630aa2694cc37f40R253-R257'><strong>Prototype key / lookup safety</strong></a><br>`Object.fromEntries` builds a plain object for `dttmLookup`. Using user-controlled column names as keys may create collisions with special keys like `__proto__`, or other inherited properties. Consider using a Map or an object with null prototype to avoid prototype pollution and accidental collisions.<br> - [ ] <a href='https://github.com/apache/superset/pull/36990/files#diff-284708d992f68953fcc53db62a3973d8182944146110ec8b630aa2694cc37f40R249-R271'><strong>Case-collisions</strong></a><br>Converting column names to lowercase intentionally collapses columns that only differ by case. This resolves the case-insensitivity bug but may hide columns that legitimately differ only by casing. Confirm this behavior is desirable across consumers and document/handle any unexpected duplicates.<br> - [ ] <a href='https://github.com/apache/superset/pull/36990/files#diff-fc79d7f1ec660a1b22f37eb1c00181fceafd31e30066c9c719830782b204361eR56-R68'><strong>Column options casing scenarios</strong></a><br>The default `options` only contains an uppercase `ORDERDATE`. The code path being fixed concerns renamed columns (different casing). Add tests that simulate the dataset options changing to a lowercase `orderdate` (i.e., the option list itself containing a different-cased column_name) to ensure visibility remains correct.<br> - [ ] <a href='https://github.com/apache/superset/pull/36990/files#diff-fc79d7f1ec660a1b22f37eb1c00181fceafd31e30066c9c719830782b204361eR70-R77'><strong>Multi-value groupby not tested</strong></a><br>The new test only asserts visibility when a single groupby value is present. In real usage, users can select multiple groupby columns; ensure the visibility remains true when a temporal column appears alongside other columns (mixed arrays). Add coverage for multi-selection cases to prevent regressions.<br> </td></tr> </table> -- 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]
