codeant-ai-for-open-source[bot] commented on PR #36896:
URL: https://github.com/apache/superset/pull/36896#issuecomment-3793871507
Question: - refactored \mixedtimeseries\ with a new helper and added tests
for those edge cases (commas, missing maps, etc.).
- hardened the custom rules script with better guards and added the
suggested cleanups in the backend and shell scripts.
everything's pushed to the branch. re-triggering the review now!
/review
Answer:
Thanks — I re-reviewed only the new/changed lines you said you pushed (the
mixed-timeseries refactor + helper/tests, the hardened check-custom-rules
script, and the small backend/shell cleanups). Overall this looks good and most
of my prior nit picks appear addressed. A short summary and a few remaining
small suggestions to consider:
What I checked and confirmed
- MixedTimeseries stackGroup
- The new helper to compute the first group-by value (uses label_map when
present, otherwise a robust fallback) is present and the new unit tests cover:
- missing label_map fallback
- commas inside a dimension value (label_map-based resolution)
- groupby length == 1 (no stackGroup)
- transformProps now passes stackGroup into transformSeries and
transformSeries uses stackGroup as the base stack id before appending the
suffix (eg 'SF\na'). Tests assert expected series[].stack values for both
queries (a/b suffixes). Good coverage.
- check-custom-rules.js
- Parser/traverse handling: you now try/catch requires and early-return
(with a clear warning) when @babel/parser or @babel/traverse are missing,
preventing downstream runtime errors. Good.
- fast-glob: you attempt fast-glob if present, fall back to core glob (or
bail with a clear warning) — this is robust and avoids failing in dev
environments without dev deps.
- Removed the hard process.exit and added informative warnings. Good.
- Backend/shell/script cleanups
- Replaced patch.dict("flask.current_app.config", ...) with
patch.dict(app.config, ...) where appropriate.
- Converted multi-patch with(...) blocks to chained context managers;
these are equivalent and clearer.
- scripts that chunk file lists (check-custom-rules.sh, oxlint.sh, and the
eslint docs hook) are updated to avoid command-line length issues on Windows.
- pre-commit hook updated to use python3 in the feature-flag extractor
entry — good for systems where python maps to Python 2.
Small remaining suggestions / sanity checks
(only small things left to consider — nothing blocking)
1) Document the newline stackIdSuffix convention
- Reason: the '\na' / '\nb' suffix is a non-obvious convention (used to
avoid collisions while preserving readable legend/tooltips). Add one-line
comment in transformProps where stackIdSuffix is assigned (and perhaps in
transformSeries) so future maintainers understand why the newline prefix is
used.
Example comment:
// Use a newline-prefixed suffix (e.g., '\na') to ensure unique stack IDs
across queries
// while minimizing visual leakage into legends/tooltips if the raw ID is
ever exposed.
2) Verify CI & test runs locally
- Run unit tests and the MixedTimeseries test suite locally or in CI to
ensure there are no unexpected type/compile warnings from the new helper and
tsconfig changes (noEmit:true added).
- Confirm frontend TypeScript build / type-check passes in CI (type-check
hook uses check-type.js).
3) check-custom-rules.js: ensure graceful fallback path messages are
CI-friendly
- The early-return warnings are helpful for local dev. If CI requires these
checks to run, make sure CI installs the dev deps (or set the script to exit
nonzero if parser/traverse are missing in CI). If you already added an
environment check or set CI to install dev deps, this is fine.
Suggested example behavior (if CI needs failure):
if (!parser || !traverse) {
console.warn('Skipping AST checks because dependencies are missing. In
CI, ensure devDependencies are installed.');
if (process.env.CI) process.exit(1);
return;
}
4) db_engine_specs/lint_metadata.py
- I noted earlier a duplicated ast.JoinedStr branch — I assume you removed
it in your cleanups. If not, please remove the duplicate to avoid confusion.
5) Tests for edge-case parsing
- Your new tests cover label_map missing and comma-in-value cases — great.
One extra edge-case to consider (optional):
- A label_map present but its first element is null/empty — ensure
computeFirstGroupValue coerces to a stable string (you already cast
String(...), good). A quick test for an empty string would guard that.
6) Shell script chunking
- The chunking approach is good. Consider making CHUNK_SIZE an
exported/visible variable near the top for easier tuning. Also double-check
behavior on Windows Git Bash vs Linux shells used in CI.
Conclusion / recommendation
- The changes implement the stacked+grouped behavior in a straightforward
way and are covered by tests for the main edge cases. The script hardening and
test fixes are useful and reduce flakiness for devs without full dev deps
installed.
- If you add the tiny documentation comment for the suffix, ensure CI runs
the test/type checks, and confirm the parser/traverse CI policy (fail vs skip),
I’m happy to approve this change.
If you want, I can propose the exact one-line comment for the newline
suffix, or a tiny tweak to check-custom-rules.js to make the CI vs local
behavior explicit (example snippet in point 3).
--
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]
