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

refactor: removed get_test_accessor and renamed vars with meaningful txt #284

Conversation

akhilender-bongirwar
Copy link
Contributor

Please be sure to look over the pull request guidelines here: https://github.com/spaceandtimelabs/sxt-proof-of-sql/blob/main/CONTRIBUTING.md#submit-pr.

Please go through the following checklist

Rationale for this change

fixes #236
/claim #236

This PR fixes Part 2 of issue #236

  • Refactor or remove get_test_accessor so that it is explicit what the tables, columns, and data types are in the SchemaAccessor. For example, the above query has a t table with the columns i, i0, and i1, all with BigInt type. These columns should also be renamed to be slightly less generic and/or more helpful.

Part 1 and 3 are already fixed in PR #261

What changes are included in this PR?

  • Removed the get_test_accessor function, replacing it with a schema-specific accessor.
  • Renamed variables like i, i0, i1, and s to more meaningful names like salary, department, tax, and name reflecting the context of the data.

Are these changes tested?

These changes are in tests.

@algora-pbc algora-pbc bot mentioned this pull request Oct 20, 2024
3 tasks
@akhilender-bongirwar akhilender-bongirwar force-pushed the refactor/remove-get-test-accessor-and-rename-vars branch from d6783ff to a924933 Compare October 20, 2024 11:14
- The leftExpr and rightExpr for the 3 tests in the logs is the same but it is still failing.
- debugging to use previous s,i,i0,i1 to check whether it is passing or failing.
@akhilender-bongirwar akhilender-bongirwar force-pushed the refactor/remove-get-test-accessor-and-rename-vars branch from e8dd3ea to e0ee44c Compare October 20, 2024 17:29
@akhilender-bongirwar
Copy link
Contributor Author

@iajoiner @JayWhite2357 please review

@akhilender-bongirwar
Copy link
Contributor Author

#284 (comment)

There are three tests:

  1. count_aggregation_always_have_integer_type
  2. group_by_expressions_are_parsed_before_an_order_by_referencing_an_aggregate_alias_result
  3. we_can_use_multiple_group_by_clauses_with_multiple_agg_and_non_agg_exprs

As you can see in the workflow logs here, even though I made changes in this commit, the tests were failing.

After carefully reading through the workflow logs, I noticed that the left: QueryExpr and right: QueryExpr were actually equal, but I was still getting an assertion error: left == right failed. This was the case for all three tests.

So, I explicitly assigned previous given names to only these tests, and after that all the tests passed successfully.

@akhilender-bongirwar
Copy link
Contributor Author

#284 (comment)

There are three tests:

1. `count_aggregation_always_have_integer_type`

2. `group_by_expressions_are_parsed_before_an_order_by_referencing_an_aggregate_alias_result`

3. `we_can_use_multiple_group_by_clauses_with_multiple_agg_and_non_agg_exprs`

As you can see in the workflow logs here, even though I made changes in this commit, the tests were failing.

After carefully reading through the workflow logs, I noticed that the left: QueryExpr and right: QueryExpr were actually equal, but I was still getting an assertion error: left == right failed. This was the case for all three tests.

So, I explicitly assigned previous given names to only these tests, and after that all the tests passed successfully.

Could you guide me on how to proceed here

@iajoiner
Copy link
Contributor

That looks really weird. Let me take a look.

- The leftExpr and rightExpr for the 3 tests in the logs is the same but it is still failing.
- debugging to use previous s,i,i0,i1 to check whether it is passing or failing.
@iajoiner iajoiner force-pushed the refactor/remove-get-test-accessor-and-rename-vars branch from 1f2da13 to 8c54c5a Compare October 22, 2024 19:54
@iajoiner
Copy link
Contributor

@akhilender-bongirwar I really doubt that's the case. Could you please fix these three tests and ping me if you get errors?

…ithub.com:akhilender-bongirwar/sxt-proof-of-sql into refactor/remove-get-test-accessor-and-rename-vars
@akhilender-bongirwar
Copy link
Contributor Author

@akhilender-bongirwar I really doubt that's the case. Could you please fix these three tests and ping me if you get errors?

Sure.

@akhilender-bongirwar
Copy link
Contributor Author

In this latest commit, I just renamed the variables to make them slightly less generic, as you can see, but the tests are still failing.

@iajoiner PTAL

@iajoiner
Copy link
Contributor

iajoiner commented Oct 23, 2024

@akhilender-bongirwar Oh I see. In these situations ordering of certain columns might be dependent on their alphabetical ordering.

Since in all these cases you have very large print outs that are only slightly different you want to do is to feed left hand side and right hand side into ChatGPT and let it summarize differences for you. Then you should solve them.

@akhilender-bongirwar akhilender-bongirwar force-pushed the refactor/remove-get-test-accessor-and-rename-vars branch from 7120962 to e493a88 Compare October 23, 2024 09:25
@akhilender-bongirwar akhilender-bongirwar force-pushed the refactor/remove-get-test-accessor-and-rename-vars branch from e493a88 to 26c26c5 Compare October 23, 2024 09:38
- group_by_expressions_are_parsed_before_an_order_by_referencing_an_aggregate_alias_result
@akhilender-bongirwar akhilender-bongirwar force-pushed the refactor/remove-get-test-accessor-and-rename-vars branch from c2a9e83 to 525b882 Compare October 23, 2024 10:06
- count_aggregation_always_have_integer_type
@akhilender-bongirwar
Copy link
Contributor Author

@iajoiner, I didn’t realize the order would cause the test to fail 😅. Passed all checks. Thanks so much for your help! 😊

Copy link
Contributor

@iajoiner iajoiner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akhilender-bongirwar Thanks for your hard work and patience! :)

@akhilender-bongirwar
Copy link
Contributor Author

@JayWhite2357 please review

@akhilender-bongirwar
Copy link
Contributor Author

@JayWhite2357 could you please review

@JayWhite2357 JayWhite2357 merged commit a0fd293 into spaceandtimelabs:main Oct 28, 2024
11 checks passed
Copy link

🎉 This PR is included in version 0.34.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@akhilender-bongirwar akhilender-bongirwar deleted the refactor/remove-get-test-accessor-and-rename-vars branch October 29, 2024 15:25
@akhilender-bongirwar
Copy link
Contributor Author

@iajoiner @JayWhite2357, I actually did not recieve the bounty till now. Could you look into it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove query! macro.
3 participants