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

feat: add binary plan normalizer #74

Merged
merged 3 commits into from
Jun 21, 2023

Conversation

EpsilonPrime
Copy link
Member

The normalizer orders the extension uri spaces and function extensions alphabetically (and renumbers accordingly) so that it is easier to compare binary plans. A slight change to the existing sort behavior is made to be consistent with this ordering.

The normalizer orders the extension uri spaces and function extensions alphabetically (and renumbers accordingly) so that it is easier to compare binary plans.   A slight change to the existing sort behavior is made to be consistent with this ordering.
Copy link
Member

@westonpace westonpace left a comment

Choose a reason for hiding this comment

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

Looks good. Probably don't need to dive too far into test internals too. It makes sense you'd want a canonical ordering.

@@ -254,7 +254,15 @@ std::string outputFunctionsSection(const SymbolTable& symbolTable) {
std::stringstream text;

std::map<uint32_t, std::string> spaceNames;
std::set<uint32_t> usedSpaces;
auto cmp = [&](uint32_t a, uint32_t b) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a small comment just explaining why we are doing this

@westonpace westonpace merged commit 4601d5c into substrait-io:main Jun 21, 2023
2 checks passed
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.

2 participants