sadpandajoe commented on PR #45435: URL: https://github.com/apache/superset/pull/45435#issuecomment-6329731707
Thanks @dhruv-techdev, the SQLite-based test works for me, so you don't need to change approach to land this. Here are a few optional options, cheapest first: 1. **Pin the expression.** Add a parametrized exact-string assertion on `HiveEngineSpec._time_grain_expressions[TimeGrain.WEEK]`, like `test_ibmi.py` does. It's a few lines and catches an accidental revert. 2. **Note the `'u'` assumption.** The `from_unixtime` stand-in returns `isoweekday()`, so the test would still pass if some Hive version formatted `'u'` differently. A short comment saying this assumes `SimpleDateFormat` semantics would make that clear. If you can run the SQL Lab query from the description on a live Hive and paste the output and Hive version, that's the real evidence. 3. **Real Hive in CI.** The `test-postgres-hive` job already starts Hive 2.3.2 from `scripts/databases/hive/docker-compose.yml`, but it only runs tests marked `chart_data_flow` or `sql_json_flow`. A small marked test that runs the WEEK expression over 14 literal days against that Hive would replace the stand-ins with the real functions. It needs a new marker and a workflow edit, so only do it if you're interested. A follow-up PR is fine. One caveat on option 3: the CI Hive is 2.3.2, which uses `SimpleDateFormat`. I haven't checked how newer Hive versions handle `'u'`, and if they differ, a separate issue would be the place to track it. -- 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]
