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

SQLServer enable filter #11471

Merged
merged 42 commits into from
Nov 7, 2024
Merged

SQLServer enable filter #11471

merged 42 commits into from
Nov 7, 2024

Conversation

AdRiley
Copy link
Member

@AdRiley AdRiley commented Nov 1, 2024

Pull Request Description

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.
  • If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,
    or the Snowflake database integration, a run of the Extra Tests has been scheduled.
    • If applicable, it is suggested to paste a link to a successful run of the Extra Tests.

@AdRiley AdRiley added the CI: No changelog needed Do not require a changelog entry for this PR. label Nov 1, 2024
@AdRiley AdRiley marked this pull request as ready for review November 4, 2024 16:24
test/Microsoft_Tests/src/SQLServer_Spec.enso Outdated Show resolved Hide resolved
test/Microsoft_Tests/src/SQLServer_Spec.enso Outdated Show resolved Hide resolved
@AdRiley AdRiley added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Nov 5, 2024
@AdRiley AdRiley force-pushed the wip/adr/sqlserver-enable-filter branch from cc870c2 to e5baf3d Compare November 5, 2024 11:36
Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

Overall shape looks great and aligned with what we discussed. Sorry if I'm asking for too many additional things if this is just an iterative improvement and supposed to be still improved in future iterations - I do understand some of my comments may just be things 'for later'.

For now:

  1. I'd like to refactor some copy pasted code as it should increase clarity and maintainability:
    • the conversions in both directions - as this will make it much clearer where we are crossing boundaries between the boolean world and value world. Also BTW I'd suggest updating the docstring of _generate_expression with this 'two kinds of values' metaphor, as IMHO it is really helpful in understanding this whole ordeal.
    • minor thing but the application of metadata to the operation that was copied from Base_Generator should also be refactored IMHO.
  2. I really don't understand why we have a separate top-level _generate_column function. As far as we discussed, it seems that there is nothing really special about a column - it is just a thing that expects a 'value level' expression and not booleans. So I'd assume I should just be able to call _generate_expression with generate_as_bool=False and it should work. If not, why?

@AdRiley AdRiley force-pushed the wip/adr/sqlserver-enable-filter branch from 52dc9ba to 1f80976 Compare November 7, 2024 09:44
@AdRiley AdRiley removed the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Nov 7, 2024
@AdRiley AdRiley merged commit 676a7d4 into develop Nov 7, 2024
36 of 37 checks passed
@AdRiley AdRiley deleted the wip/adr/sqlserver-enable-filter branch November 7, 2024 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants