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(plan,expr): use Expressions fields in VirtualTable instead of Literal #63

Merged

Conversation

anshuldata
Copy link
Contributor

@anshuldata anshuldata commented Oct 30, 2024

  • This change migrates VirtualTable to not use deprecated field and instead use newly introduced field in this PR
  • Fix VirtualTable proto to generate Expression Struct instead of Literal Struct
  • This PR doesn't have breaking change
  • This PR doesn't introduce PublicAPI to use Expression in building VirtualTableRelation. I will introduce it as separate PR

Copy link

codecov bot commented Oct 30, 2024

Codecov Report

Attention: Patch coverage is 51.02041% with 24 lines in your changes missing coverage. Please review.

Project coverage is 56.51%. Comparing base (833624e) to head (3be7754).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
plan/internal/helper.go 40.00% 8 Missing and 1 partial ⚠️
expr/expression.go 0.00% 8 Missing ⚠️
plan/plan.go 53.84% 4 Missing and 2 partials ⚠️
plan/relations.go 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #63      +/-   ##
==========================================
- Coverage   56.56%   56.51%   -0.06%     
==========================================
  Files          38       39       +1     
  Lines        8268     8310      +42     
==========================================
+ Hits         4677     4696      +19     
- Misses       3392     3412      +20     
- Partials      199      202       +3     

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

@anshuldata anshuldata force-pushed the anshul/VirtualTableMigrateToNewField branch from 6c86353 to c1b4d2a Compare October 30, 2024 11:49
@anshuldata anshuldata changed the title fix code to use Expressions fields in VirtualTable instead of Literal <DRAFT>fix code to use Expressions fields in VirtualTable instead of Literal Nov 4, 2024
@anshuldata
Copy link
Contributor Author

Converting this PR to draft. I will raise separate PR for proto update. I will rebase and undraft this PR once proto PR is merged

@anshuldata anshuldata marked this pull request as draft November 4, 2024 07:09
@EpsilonPrime
Copy link
Member

This will remove the literal support so consuming older plans won't work but that should be okay given the current usage of this feature.

@anshuldata anshuldata force-pushed the anshul/VirtualTableMigrateToNewField branch 2 times, most recently from 55ff889 to 3b6d780 Compare November 5, 2024 11:11
@anshuldata anshuldata changed the title <DRAFT>fix code to use Expressions fields in VirtualTable instead of Literal fix code to use Expressions fields in VirtualTable instead of Literal Nov 5, 2024
@anshuldata anshuldata force-pushed the anshul/VirtualTableMigrateToNewField branch from 3b6d780 to 60578ef Compare November 5, 2024 11:12
@anshuldata anshuldata marked this pull request as ready for review November 5, 2024 11:12
@anshuldata
Copy link
Contributor Author

This will remove the literal support so consuming older plans won't work but that should be okay given the current usage of this feature.

@EpsilonPrime this PR does consume older plans. I have added this test case to make sure this. Let me know if I am missing something

expr/literals.go Show resolved Hide resolved
@zeroshade zeroshade changed the title fix code to use Expressions fields in VirtualTable instead of Literal fix(plan,expr): code to use Expressions fields in VirtualTable instead of Literal Nov 8, 2024
@zeroshade zeroshade changed the title fix(plan,expr): code to use Expressions fields in VirtualTable instead of Literal fix(plan,expr): use Expressions fields in VirtualTable instead of Literal Nov 8, 2024
@anshuldata
Copy link
Contributor Author

anshuldata commented Nov 8, 2024

@zeroshade we need to change github repository setting to not allow merge if change is not approved
Screenshot 2024-11-08 at 5 33 20 PM

We need to add "Branch protection rules" rules as below (Settings --> branches --> Branch protection rules)

Screenshot 2024-11-08 at 5 41 00 PM

@zeroshade
Copy link
Contributor

@anshuldata I don't have the permissions for that.

@jacques-n could you make the suggested change to the branch protection rules?

@jacques-n
Copy link
Contributor

could you make the suggested change to the branch protection rules?

It looks like it is already added. Maybe someone else took the initiative? Or I'm looking in the wrong place...

@EpsilonPrime
Copy link
Member

I added them. Looks like we need some added test coverage to merge this PR.

@anshuldata
Copy link
Contributor Author

anshuldata commented Nov 13, 2024

Looks like we need some added test coverage to merge this PR.

code coverage doesn't recognize coverage in tests across package. So test in plan_test/plan_builder_test coverage which covers expression.go/helper.go changes is not accounted.
I need to figure out coverage option to detect it

@anshuldata anshuldata merged commit 08fa953 into substrait-io:main Nov 13, 2024
6 of 8 checks passed
@anshuldata anshuldata deleted the anshul/VirtualTableMigrateToNewField branch November 13, 2024 05:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants