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

Introduce Value_Type.Null, fix edge cases for read_many #11737

Open
wants to merge 44 commits into
base: develop
Choose a base branch
from

Conversation

radeusgd
Copy link
Member

@radeusgd radeusgd commented Dec 2, 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.

@radeusgd radeusgd added the CI: No changelog needed Do not require a changelog entry for this PR. label Dec 2, 2024
@radeusgd radeusgd self-assigned this Dec 2, 2024
@radeusgd radeusgd force-pushed the wip/radeusgd/6281-value-type-null branch 3 times, most recently from c57057b to 22a2371 Compare December 3, 2024 17:22
@radeusgd radeusgd marked this pull request as ready for review December 3, 2024 17:23
@radeusgd
Copy link
Member Author

radeusgd commented Dec 3, 2024

Changes affect Snowflake so I'm running the Extra suite:

I expect it to fail because it is failing on develop. Yet I want to see if there are no more failures than what is expected.

@radeusgd
Copy link
Member Author

radeusgd commented Dec 3, 2024

Followup to fix Nothing * Nothing: #11751 and Nothing in expressions in DB #11757.

@@ -628,6 +626,9 @@ make_custom_cast column target_value_type type_mapping =
result = Ref.new Nothing
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this logic hard to follow. The return value would be either Nothing or the return value of result.put expr -- but isn't the the return value of Ref.put the old value?

Copy link
Member Author

@radeusgd radeusgd Dec 4, 2024

Choose a reason for hiding this comment

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

The return value of put is discarded.

The function returns ref.get at the very end.

I agree it is not ideal. This is a hack to get multiline if without adding a ton of indentation to the else branches. There's just too many branches here (and while sometimes too many branches is bad, here it is just intrinsic to this function) to make it readable.

Once we get multiline if (#6408) I will be more than happy to replace this with just:

if cond1 then
    ...
else if cond2 then
    ...
else
    Nothing

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think I should rewrite it with increasing indentation?

I'd rather keep it as is as IMO it is readable once the reasons are explained.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, it makes more sense now.

@radeusgd
Copy link
Member Author

radeusgd commented Dec 19, 2024

Followup to fix Nothing * Nothing: #11751 and Nothing in expressions in DB #11757.

I think we could merge this PR and then improve the missing bits from these 2 tickets in a follow-up PR. Unless there is any opposing views and we want to do it as a single change?

@radeusgd radeusgd force-pushed the wip/radeusgd/6281-value-type-null branch from cb8fc47 to 2b94b24 Compare December 19, 2024 14:13
@radeusgd
Copy link
Member Author

I've rebased on latest develop and noticed there are some test failures because of other changes. I will try to fix these soon, but it shouldn't change the shape of the overall changes that much.

@@ -628,6 +626,9 @@ make_custom_cast column target_value_type type_mapping =
result = Ref.new Nothing
Copy link
Contributor

Choose a reason for hiding this comment

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

I see, it makes more sense now.

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.

Introduce a Value_Type.Null.
2 participants