From 55538d9e28f4301fd242df7cec2142ab1bfd92b1 Mon Sep 17 00:00:00 2001 From: Bhavna Jindal Date: Mon, 24 Jul 2023 12:44:49 -0700 Subject: [PATCH 1/2] I2527: Fixed NaN issue in variance functions --- .../KQLAggregationFunctions.cpp | 20 +++++++++++-- .../KQL/gtest_KQL_AggregateFunctions.cpp | 30 +++++++++++-------- 2 files changed, 35 insertions(+), 15 deletions(-) diff --git a/src/Parsers/Kusto/KustoFunctions/KQLAggregationFunctions.cpp b/src/Parsers/Kusto/KustoFunctions/KQLAggregationFunctions.cpp index 2a5cae244fec..8ec3992d43de 100644 --- a/src/Parsers/Kusto/KustoFunctions/KQLAggregationFunctions.cpp +++ b/src/Parsers/Kusto/KustoFunctions/KQLAggregationFunctions.cpp @@ -599,7 +599,14 @@ bool TakeAnyIf::convertImpl(String & out, IParser::Pos & pos) bool Variance::convertImpl(String & out, IParser::Pos & pos) { - return directMapping(out, pos, "varSamp"); + const String fn_name = getKQLFunctionName(pos); + if (fn_name.empty()) + return false; + + const String expr = getArgument(fn_name, pos); + out = std::format("IF (isNaN(varSamp({0}) AS variance_{1}), 0, variance_{1})", expr, generateUniqueIdentifier()); + + return true; } bool VarianceIf::convertImpl(String & out, IParser::Pos & pos) @@ -610,14 +617,21 @@ bool VarianceIf::convertImpl(String & out, IParser::Pos & pos) const String expr = getArgument(fn_name, pos); const String predicate = getArgument(fn_name, pos); - out = std::format("varSampIf({}, {})", expr, predicate); + out = std::format("IF (isNaN(varSampIf({0}, {1}) AS variance_{2}), 0, variance_{2})", expr, predicate, generateUniqueIdentifier()); return true; } bool VarianceP::convertImpl(String & out, IParser::Pos & pos) { - return directMapping(out, pos, "varPop"); + const String fn_name = getKQLFunctionName(pos); + if (fn_name.empty()) + return false; + + const String expr = getArgument(fn_name, pos); + out = std::format("IF (isNaN(varPop({0}) AS variance_{1}), 0, variance_{1})", expr, generateUniqueIdentifier()); + + return true; } bool CountDistinct::convertImpl(String & out, IParser::Pos & pos) diff --git a/src/Parsers/tests/KQL/gtest_KQL_AggregateFunctions.cpp b/src/Parsers/tests/KQL/gtest_KQL_AggregateFunctions.cpp index 5cb5d5e46cca..ba3f669788a2 100644 --- a/src/Parsers/tests/KQL/gtest_KQL_AggregateFunctions.cpp +++ b/src/Parsers/tests/KQL/gtest_KQL_AggregateFunctions.cpp @@ -110,18 +110,6 @@ INSTANTIATE_TEST_SUITE_P(ParserKQLQuery_Aggregate, ParserTest, "Customers | summarize by FirstName, LastName, Age", "SELECT\n FirstName,\n LastName,\n Age\nFROM Customers\nGROUP BY\n FirstName,\n LastName,\n Age" }, - { - "Customers | summarize variance(Age)", - "SELECT varSamp(Age) AS variance_Age\nFROM Customers" - }, - { - "Customers | summarize variancep(Age)", - "SELECT varPop(Age) AS variancep_Age\nFROM Customers" - }, - { - "Customers | summarize varianceif(Age, Age < 30)", - "SELECT varSampIf(Age, Age < 30) AS varianceif_Age\nFROM Customers" - }, { "Customers | summarize z=arg_max(Age, FirstName, LastName) by Occupation", "SELECT\n Occupation,\n argMax(FirstName, Age) AS FirstName,\n argMax(LastName, Age) AS LastName,\n argMax(Age, Age) AS z\nFROM Customers\nGROUP BY Occupation" @@ -135,3 +123,21 @@ INSTANTIATE_TEST_SUITE_P(ParserKQLQuery_Aggregate, ParserTest, "SELECT uniqCombined64Merge(18)(xy) AS Column1\nFROM\n(\n SELECT uniqCombined64MergeState(18)(arrayJoin([x, y])) AS xy\n FROM\n (\n SELECT\n uniqCombined64State(18)(Education) AS x,\n uniqCombined64State(18)(Occupation) AS y\n FROM Customers\n )\n)" } }))); + +INSTANTIATE_TEST_SUITE_P(ParserKQLQuery_Aggregate, ParserRegexTest, + ::testing::Combine( + ::testing::Values(std::make_shared()), + ::testing::ValuesIn(std::initializer_list{ + { + "Customers | summarize variance(Age)", + R"(SELECT IF\(isNaN\(varSamp\(Age\) AS variance_\d+\), 0, variance_\d+\) AS variance_Age\nFROM Customers)" + }, + { + "Customers | summarize variancep(Age)", + R"(SELECT IF\(isNaN\(varPop\(Age\) AS variance_\d+\), 0, variance_\d+\) AS variancep_Age\nFROM Customers)" + }, + { + "Customers | summarize varianceif(Age, Age < 30)", + R"(SELECT IF\(isNaN\(varSampIf\(Age, Age < 30\) AS variance_\d+\), 0, variance_\d+\) AS varianceif_Age\nFROM Customers)" + } +}))); From 8ec8acde85dab857c97006bcf1937ded424355bd Mon Sep 17 00:00:00 2001 From: Bhavna Jindal Date: Mon, 31 Jul 2023 11:34:42 -0700 Subject: [PATCH 2/2] Fixed 2518 --- .../KQLAggregationFunctions.cpp | 19 ++++++++++++++++--- .../KQL/gtest_KQL_AggregateFunctions.cpp | 6 +++--- .../0_stateless/02366_kql_summarize.sql | 3 +++ 3 files changed, 22 insertions(+), 6 deletions(-) diff --git a/src/Parsers/Kusto/KustoFunctions/KQLAggregationFunctions.cpp b/src/Parsers/Kusto/KustoFunctions/KQLAggregationFunctions.cpp index 8ec3992d43de..0a21c5da031f 100644 --- a/src/Parsers/Kusto/KustoFunctions/KQLAggregationFunctions.cpp +++ b/src/Parsers/Kusto/KustoFunctions/KQLAggregationFunctions.cpp @@ -604,7 +604,11 @@ bool Variance::convertImpl(String & out, IParser::Pos & pos) return false; const String expr = getArgument(fn_name, pos); - out = std::format("IF (isNaN(varSamp({0}) AS variance_{1}), 0, variance_{1})", expr, generateUniqueIdentifier()); + out = std::format( + "IF (isNaN(varSamp(if(toTypeName({0}) = 'Nullable(Nothing)', throwIf(toTypeName({0}) = 'Nullable(Nothing)', " + "'summarize operator: Failed to resolve scalar expression named null'), {0})) AS variance_{1}), 0, variance_{1})", + expr, + generateUniqueIdentifier()); return true; } @@ -617,7 +621,12 @@ bool VarianceIf::convertImpl(String & out, IParser::Pos & pos) const String expr = getArgument(fn_name, pos); const String predicate = getArgument(fn_name, pos); - out = std::format("IF (isNaN(varSampIf({0}, {1}) AS variance_{2}), 0, variance_{2})", expr, predicate, generateUniqueIdentifier()); + out = std::format( + "IF (isNaN(varSampIf((if(toTypeName({0}) = 'Nullable(Nothing)', throwIf(toTypeName({0}) = 'Nullable(Nothing)', " + "'summarize operator: Failed to resolve scalar expression named null'), {0})), {1}) AS variance_{2}), 0, variance_{2})", + expr, + predicate, + generateUniqueIdentifier()); return true; } @@ -629,7 +638,11 @@ bool VarianceP::convertImpl(String & out, IParser::Pos & pos) return false; const String expr = getArgument(fn_name, pos); - out = std::format("IF (isNaN(varPop({0}) AS variance_{1}), 0, variance_{1})", expr, generateUniqueIdentifier()); + out = std::format( + "IF (isNaN(varPop(if(toTypeName({0}) = 'Nullable(Nothing)', throwIf(toTypeName({0}) = 'Nullable(Nothing)', " + "'summarize operator: Failed to resolve scalar expression named null'), {0})) AS variance_{1}), 0, variance_{1})", + expr, + generateUniqueIdentifier()); return true; } diff --git a/src/Parsers/tests/KQL/gtest_KQL_AggregateFunctions.cpp b/src/Parsers/tests/KQL/gtest_KQL_AggregateFunctions.cpp index ba3f669788a2..173b8ea789aa 100644 --- a/src/Parsers/tests/KQL/gtest_KQL_AggregateFunctions.cpp +++ b/src/Parsers/tests/KQL/gtest_KQL_AggregateFunctions.cpp @@ -130,14 +130,14 @@ INSTANTIATE_TEST_SUITE_P(ParserKQLQuery_Aggregate, ParserRegexTest, ::testing::ValuesIn(std::initializer_list{ { "Customers | summarize variance(Age)", - R"(SELECT IF\(isNaN\(varSamp\(Age\) AS variance_\d+\), 0, variance_\d+\) AS variance_Age\nFROM Customers)" + R"(SELECT IF\(isNaN\(varSamp\(if\(toTypeName\(Age\) = \'Nullable\(Nothing\)\', throwIf\(toTypeName\(Age\) = \'Nullable\(Nothing\)\', \'summarize operator: Failed to resolve scalar expression named null\'\), Age\)\) AS variance_\d+\), 0, variance_\d+\) AS variance_Age\nFROM Customers)" }, { "Customers | summarize variancep(Age)", - R"(SELECT IF\(isNaN\(varPop\(Age\) AS variance_\d+\), 0, variance_\d+\) AS variancep_Age\nFROM Customers)" + R"(SELECT IF\(isNaN\(varPop\(if\(toTypeName\(Age\) = \'Nullable\(Nothing\)\', throwIf\(toTypeName\(Age\) = \'Nullable\(Nothing\)\', \'summarize operator: Failed to resolve scalar expression named null\'\), Age\)\) AS variance_\d+\), 0, variance_\d+\) AS variancep_Age\nFROM Customers)" }, { "Customers | summarize varianceif(Age, Age < 30)", - R"(SELECT IF\(isNaN\(varSampIf\(Age, Age < 30\) AS variance_\d+\), 0, variance_\d+\) AS varianceif_Age\nFROM Customers)" + R"(SELECT IF\(isNaN\(varSampIf\(if\(toTypeName\(Age\) = \'Nullable\(Nothing\)\', throwIf\(toTypeName\(Age\) = \'Nullable\(Nothing\)\', \'summarize operator: Failed to resolve scalar expression named null\'\), Age\), Age < 30\) AS variance_\d+\), 0, variance_\d+\) AS varianceif_Age\nFROM Customers)" } }))); diff --git a/tests/queries/0_stateless/02366_kql_summarize.sql b/tests/queries/0_stateless/02366_kql_summarize.sql index 1c260e548fd6..111dcee793d3 100644 --- a/tests/queries/0_stateless/02366_kql_summarize.sql +++ b/tests/queries/0_stateless/02366_kql_summarize.sql @@ -132,6 +132,9 @@ print '-- variance/variancep/varianceif --'; Customers | summarize variance(Age); Customers | summarize variancep(Age); Customers | summarize varianceif(Age, Age < 30); +Customers | summarize variance(null); -- { clientError Code: 395 } +Customers | summarize variancep(null); -- { clientError Code: 395 } +Customers | summarize varianceif(null, Age < 30); -- { clientError Code: 395 } print '-- arg_max --'; Customers | summarize arg_max(Age); -- { clientError NUMBER_OF_ARGUMENTS_DOESNT_MATCH }