From 3de54baf6201c0cff2f3609a7cb564b31839e4bc Mon Sep 17 00:00:00 2001 From: David Sisson Date: Sat, 24 Jun 2023 01:30:40 -0700 Subject: [PATCH 1/3] refactor: upgrade multi-key sort as suggested by @bkietz --- .../converter/ReferenceNormalizer.cpp | 34 +++++++------------ 1 file changed, 13 insertions(+), 21 deletions(-) diff --git a/src/substrait/textplan/converter/ReferenceNormalizer.cpp b/src/substrait/textplan/converter/ReferenceNormalizer.cpp index 38c4d209..17aee725 100644 --- a/src/substrait/textplan/converter/ReferenceNormalizer.cpp +++ b/src/substrait/textplan/converter/ReferenceNormalizer.cpp @@ -14,27 +14,19 @@ namespace { bool compareExtensionFunctions( const ::substrait::proto::extensions::SimpleExtensionDeclaration& a, const ::substrait::proto::extensions::SimpleExtensionDeclaration& b) { - // First sort so that extension functions proceed any other kind of extension. - if (a.has_extension_function() && !b.has_extension_function()) { - return true; - } else if (!a.has_extension_function() && b.has_extension_function()) { - // Extension functions always come first. - return false; - } else if (!a.has_extension_function() && !b.has_extension_function()) { - // Both are not extension functions, no difference in ordering. - return false; - } - // Now sort by space. - if (a.extension_function().extension_uri_reference() < - b.extension_function().extension_uri_reference()) { - return true; - } else if ( - a.extension_function().extension_uri_reference() > - b.extension_function().extension_uri_reference()) { - return false; - } - // Finally sort by name within a space. - return a.extension_function().name() < b.extension_function().name(); + auto ord = [](const auto& decl) { + return make_tuple( + // First sort so that extension functions precede any other kind of + // extension. + not decl.has_extension_function(), + // Next sort by space. + decl.extension_function().extension_uri_reference(), + // Finally sort by name within a space. + decl.extension_function().name()); + }; + + // Now let the default tuple compare do the rest of the work. + return ord(a) > ord(b); } void normalizeFunctionsForExpression( From 8cb481ffe50d7db55c59233c77f9efc0ff7df31c Mon Sep 17 00:00:00 2001 From: David Sisson Date: Mon, 26 Jun 2023 08:33:41 -0700 Subject: [PATCH 2/3] Update src/substrait/textplan/converter/ReferenceNormalizer.cpp Co-authored-by: Benjamin Kietzman --- src/substrait/textplan/converter/ReferenceNormalizer.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/substrait/textplan/converter/ReferenceNormalizer.cpp b/src/substrait/textplan/converter/ReferenceNormalizer.cpp index 17aee725..562804a1 100644 --- a/src/substrait/textplan/converter/ReferenceNormalizer.cpp +++ b/src/substrait/textplan/converter/ReferenceNormalizer.cpp @@ -20,9 +20,9 @@ bool compareExtensionFunctions( // extension. not decl.has_extension_function(), // Next sort by space. - decl.extension_function().extension_uri_reference(), + std::string_view{decl.extension_function().extension_uri_reference()}, // Finally sort by name within a space. - decl.extension_function().name()); + std::string_view{decl.extension_function().name()}); }; // Now let the default tuple compare do the rest of the work. From 9ca7847540c81c77e698d0abd46d154ef5bd1a8e Mon Sep 17 00:00:00 2001 From: David Sisson Date: Mon, 26 Jun 2023 09:04:31 -0700 Subject: [PATCH 3/3] Only wrap the strings with string_view. --- src/substrait/textplan/converter/ReferenceNormalizer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/substrait/textplan/converter/ReferenceNormalizer.cpp b/src/substrait/textplan/converter/ReferenceNormalizer.cpp index 562804a1..f2c7d849 100644 --- a/src/substrait/textplan/converter/ReferenceNormalizer.cpp +++ b/src/substrait/textplan/converter/ReferenceNormalizer.cpp @@ -20,7 +20,7 @@ bool compareExtensionFunctions( // extension. not decl.has_extension_function(), // Next sort by space. - std::string_view{decl.extension_function().extension_uri_reference()}, + decl.extension_function().extension_uri_reference(), // Finally sort by name within a space. std::string_view{decl.extension_function().name()}); };