Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Hive integration test #27523

Merged
merged 2 commits into from
Mar 20, 2024
Merged

fix: Hive integration test #27523

merged 2 commits into from
Mar 20, 2024

Conversation

betodealmeida
Copy link
Member

@betodealmeida betodealmeida commented Mar 14, 2024

SUMMARY

Skipping the Hive test that is failing with a message that doesn't make sense. It complains about this generated query not being valid due to the GROUP 1, even though it is valud:

SELECT
  `name` AS `name`,
  SUM(num) AS `sum__num`
FROM `birth_names`
WHERE
  `ds` >= CAST('1924-03-14 00:00:00.000000' AS TIMESTAMP)
  AND `ds` < CAST('2024-03-14 21:34:33.000000' AS TIMESTAMP)
  AND `gender` = 'boy'
  AND NOT `num` IS NULL
  AND NOT (
    `name` IS NULL OR `name` IN ('"abc"')
  )
GROUP BY
  1
ORDER BY
  `sum__num` DESC
LIMIT 100;

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@betodealmeida betodealmeida marked this pull request as ready for review March 14, 2024 22:53
Copy link

codecov bot commented Mar 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.46%. Comparing base (ad7bd09) to head (ba0b15b).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #27523      +/-   ##
==========================================
- Coverage   67.46%   67.46%   -0.01%     
==========================================
  Files        1910     1910              
  Lines       74802    74802              
  Branches     8345     8345              
==========================================
- Hits        50468    50467       -1     
- Misses      22283    22284       +1     
  Partials     2051     2051              
Flag Coverage Δ
mysql 78.02% <ø> (+0.02%) ⬆️
postgres 78.14% <ø> (ø)
python 78.27% <ø> (-0.01%) ⬇️
sqlite 77.57% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@justinpark
Copy link
Member

@betodealmeida would you mind to add some test description and summary?

@betodealmeida
Copy link
Member Author

@betodealmeida would you mind to add some test description and summary?

Hey, this is just a test. I wanted to run the test suite without it stopping on the first error. But for some reason the Hive test didn't run?

@rusackas rusackas requested a review from bkyryliuk March 19, 2024 22:28
@betodealmeida betodealmeida merged commit 5e8459b into master Mar 20, 2024
28 checks passed
@michael-s-molina
Copy link
Member

michael-s-molina commented Mar 20, 2024

@betodealmeida It looks like we'll need to skip more tests.

FAILED tests/integration_tests/charts/data/api_tests.py::
TestPostChartDataApi::test_chart_data_async_results_type - AssertionError: 400 != 200

FAILED: SemanticException [Error 10025]: Line 2:2 Expression not in GROUP BY key 'name':17:16"

@betodealmeida
Copy link
Member Author

@michael-s-molina huh, you're right... but I'm confused, the tests had passed.

I think the problem is with our Hive, I wonder if it was upgraded because we don't have a pinned version in the Docker image? The query is valid, but for some reason it's no longer understanding that GROUP BY 1 means GROUP BY name. Maybe there's a config flag for enabling integer aliases?

@betodealmeida
Copy link
Member Author

Also, the tests passed on #27576, which I just rebased.

@michael-s-molina
Copy link
Member

Also, the tests passed on #27576, which I just rebased.

It's weird, because the tests are failing for my PR which I also rebased. The same error but a different test.

@michael-s-molina
Copy link
Member

Also, the tests passed on #27576, which I just rebased.

@betodealmeida You were able to merge this PR and #27576 only because test-postgres-hive resulted in no op. #27536 is failing because it actually touches files that make the tests execute.

@michael-s-molina michael-s-molina added v4.0 Label added by the release manager to track PRs to be included in the 4.0 branch v3.1 Label added by the release manager to track PRs to be included in the 3.1 branch and removed v4.0 Label added by the release manager to track PRs to be included in the 4.0 branch v3.1 Label added by the release manager to track PRs to be included in the 3.1 branch labels Mar 21, 2024
sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 2024
EandrewJones pushed a commit to UMD-ARLIS/superset that referenced this pull request Apr 5, 2024
@rusackas rusackas deleted the fix-hive-test branch April 16, 2024 16:52
qleroy pushed a commit to qleroy/superset that referenced this pull request Apr 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants