codeant-ai-for-open-source[bot] commented on PR #36679: URL: https://github.com/apache/superset/pull/36679#issuecomment-3661626612
## 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/36679/files#diff-b6a186993d7414cb29c7015ca427727a7191ad89ade1714cf1c659a38eae81ebR239-R260'><strong>Empty-string Normalization</strong></a><br>Exported datasets sometimes contain empty strings for optional fields. The `fix_extra` pre-load hook normalizes `extra`, but `datetime_format` (and `python_date_format`) may be empty strings and should be coerced to `None` during import to avoid surprising downstream logic.<br> - [ ] <a href='https://github.com/apache/superset/pull/36679/files#diff-b6a186993d7414cb29c7015ca427727a7191ad89ade1714cf1c659a38eae81ebR259-R260'><strong>Missing Validation</strong></a><br>The new `datetime_format` field on the import column schema has no validators. The `DatasetColumnsPutSchema` uses `Length` and `validate_python_date_format`, so the import schema is now accepting values that would be rejected later or cause runtime parsing errors. Add the same validation to keep import behavior consistent and to reject invalid formats early.<br> - [ ] <a href='https://github.com/apache/superset/pull/36679/files#diff-6661a283bfedcc486b1b2caacb933f10c9ad5441238d7bf5110df7cc1270c996R32-R39'><strong>Import-time side effects</strong></a><br>Several heavy application imports (e.g., `import_dataset`, `SqlaTable`, `Database`, security manager) were moved from local imports inside test functions to module-level imports. That can cause expensive import-time work or side effects during pytest collection (DB/model registration, app context requirements) and may make tests slower or flaky in different environments.<br> - [ ] <a href='https://github.com/apache/superset/pull/36679/files#diff-6661a283bfedcc486b1b2caacb933f10c9ad5441238d7bf5110df7cc1270c996R46-R48'><strong>Fixture coupling</strong></a><br>The tests now import `dataset_config` from integration tests as `dataset_fixture`. These integration fixtures may introduce coupling to slower resources or assumptions about fixture shape. Confirm the fixture is self-contained for unit tests and not mutated across tests (deepcopy is used in some cases, but verify all usages).<br> - [ ] <a href='https://github.com/apache/superset/pull/36679/files#diff-6661a283bfedcc486b1b2caacb933f10c9ad5441238d7bf5110df7cc1270c996R674-R702'><strong>Schema validation risk</strong></a><br>The new test `test_import_dataset_column_datetime_format` exercises that `datetime_format` is preserved after import. Ensure the import schema and code changes that accept `datetime_format` exist and perform decoding before calling `import_dataset`. If the schema change is missing, this test will fail. Also consider asserting the schema load step preserved the field to isolate failures.<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]
