aminghadersohi commented on PR #44695: URL: https://github.com/apache/superset/pull/44695#issuecomment-5902134724
@rebenitez1802 All four nits are addressed; none are deferred. Follow-up commit: ddfec16e01a4f603476c49bdf41d61a48bbf884c. 1. **Timezone-aware timestamps — fixed in ddfec16e01.** `to_json_value` converts aware timestamps to UTC and removes the offset before producing the `USER_ENTERED` datetime string; naive values keep their clock time (`superset/db_engine_specs/gsheets.py:94-104`). Documented with a date-boundary example in `docs/admin_docs/configuration/google-sheets.mdx`. Scalar and upload-payload tests cover UTC, positive/negative offsets, a half-hour offset, microseconds, nulls, and unchanged naive datetimes. 2. **Nullable Int64 — fixed in ddfec16e01.** Cell conversion happens outside pandas' dtype inference, preserving integer JSON values and empty strings for missing cells (`gsheets.py:645-651`). `test_upload_cell_types[nullable-int]` asserts exact JSON value types, including an integer above 2^53 to catch accidental float conversion. 3. **Time/edit-path coverage — completed in ddfec16e01, extending 4954f82e99.** Upload tests cover `datetime.time` with microseconds, midnight, and a missing cell. The create/edit subject regression covers serialized/dictionary credentials, omitted/false/true impersonation flags, and catalogs at both supported payload locations. Validation sends no subject in all cases. The real-engine DBAPI test continues to verify that the service-account query path also sends no subject. 4. **DataFrame/list variable reuse — fixed in ddfec16e01.** `normalized_df` is the DataFrame and `values` is the row list (`gsheets.py:645-655`). Upload tests check the resulting request payload and that the source DataFrame is unchanged. 5. **Delegation/create-flow decision — closed in 4954f82e9910ecc6d9ca0ae13821f6c95537582a; expanded regression coverage in ddfec16e01.** Keep validation aligned with service-account queries on both create and edit, rather than enabling delegation implicitly. Query-time encrypted-extra adapter arguments replace the URL subject (`superset/models/core.py:685-694`, `gsheets.py:380-392`), so the two-step empty-create/edit workaround does not confer the admin's Google identity. Private sheets must be shared with the service account; the limitation is documented. Also reverified Bito's nanosecond-duration finding: the existing `np.timedelta64` → `pd.Timedelta` branch precedes generic `.item()`, and this commit adds upload-payload coverage for 5 ns, 90 seconds represented in ns, and missing durations. Validation: **78 GSheets unit tests passed**; **all 12 applicable pre-commit hooks passed**, including mypy. The timezone/nullable-int regressions failed against the preceding code and pass with this change. No frontend changes or live Google/browser testing. -- 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]
