From ee0b2f0ad786390608d8476215b5be203aab9a1a Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Fri, 1 Nov 2024 13:37:23 +0100 Subject: [PATCH 1/3] SQL expression tree to string: add missing parenthesis that could make further evaluation of operator priority wrong --- autotest/cpp/test_ogr_swq.cpp | 9 ++--- ogr/swq_expr_node.cpp | 72 ++++++++++++++++++----------------- 2 files changed, 41 insertions(+), 40 deletions(-) diff --git a/autotest/cpp/test_ogr_swq.cpp b/autotest/cpp/test_ogr_swq.cpp index d10ca8c929d8..483ce55fa113 100644 --- a/autotest/cpp/test_ogr_swq.cpp +++ b/autotest/cpp/test_ogr_swq.cpp @@ -138,11 +138,10 @@ class PushNotOperationDownToStackFixture std::make_tuple("NOT(1 <= 2)", "1 > 2"), std::make_tuple("NOT(1 < 2)", "1 >= 2"), std::make_tuple("NOT(NOT(1))", "1"), - std::make_tuple("NOT(1 AND 2)", "(NOT (1)) OR (NOT (2))"), - std::make_tuple("NOT(1 OR 2)", "(NOT (1)) AND (NOT (2))"), - std::make_tuple("3 AND NOT(1 OR 2)", - "3 AND ((NOT (1)) AND (NOT (2)))"), - std::make_tuple("NOT(NOT(1 = 2) OR 2)", "(1 = 2) AND (NOT (2))"), + std::make_tuple("NOT(1 AND 2)", "(NOT 1) OR (NOT 2)"), + std::make_tuple("NOT(1 OR 2)", "(NOT 1) AND (NOT 2)"), + std::make_tuple("3 AND NOT(1 OR 2)", "3 AND ((NOT 1) AND (NOT 2))"), + std::make_tuple("NOT(NOT(1 = 2) OR 2)", "(1 = 2) AND (NOT 2)"), std::make_tuple("1", "1"), }; } diff --git a/ogr/swq_expr_node.cpp b/ogr/swq_expr_node.cpp index feb0124ecdc1..3cbedff6785e 100644 --- a/ogr/swq_expr_node.cpp +++ b/ogr/swq_expr_node.cpp @@ -664,6 +664,21 @@ CPLString swq_expr_node::UnparseOperationFromUnparsedSubExpr(char **apszSubExpr) return osExpr; } + const auto AddSubExpr = [this, apszSubExpr, &osExpr](int idx) + { + if (papoSubExpr[idx]->eNodeType == SNT_COLUMN || + papoSubExpr[idx]->eNodeType == SNT_CONSTANT) + { + osExpr += apszSubExpr[idx]; + } + else + { + osExpr += '('; + osExpr += apszSubExpr[idx]; + osExpr += ')'; + } + }; + switch (nOperation) { // Binary infix operators. @@ -683,63 +698,52 @@ CPLString swq_expr_node::UnparseOperationFromUnparsedSubExpr(char **apszSubExpr) case SWQ_DIVIDE: case SWQ_MODULUS: CPLAssert(nSubExprCount >= 2); - if (papoSubExpr[0]->eNodeType == SNT_COLUMN || - papoSubExpr[0]->eNodeType == SNT_CONSTANT) - { - osExpr += apszSubExpr[0]; - } - else - { - osExpr += "("; - osExpr += apszSubExpr[0]; - osExpr += ")"; - } + AddSubExpr(0); osExpr += " "; osExpr += poOp->pszName; osExpr += " "; - if (papoSubExpr[1]->eNodeType == SNT_COLUMN || - papoSubExpr[1]->eNodeType == SNT_CONSTANT) - { - osExpr += apszSubExpr[1]; - } - else - { - osExpr += "("; - osExpr += apszSubExpr[1]; - osExpr += ")"; - } + AddSubExpr(1); if ((nOperation == SWQ_LIKE || nOperation == SWQ_ILIKE) && nSubExprCount == 3) - osExpr += CPLSPrintf(" ESCAPE (%s)", apszSubExpr[2]); + { + osExpr += " ESCAPE "; + AddSubExpr(2); + } break; case SWQ_NOT: CPLAssert(nSubExprCount == 1); - osExpr.Printf("NOT (%s)", apszSubExpr[0]); + osExpr = "NOT "; + AddSubExpr(0); break; case SWQ_ISNULL: CPLAssert(nSubExprCount == 1); - osExpr.Printf("%s IS NULL", apszSubExpr[0]); + AddSubExpr(0); + osExpr += " IS NULL"; break; case SWQ_IN: - osExpr.Printf("%s IN (", apszSubExpr[0]); + AddSubExpr(0); + osExpr += " IN("; for (int i = 1; i < nSubExprCount; i++) { if (i > 1) osExpr += ","; - osExpr += "("; - osExpr += apszSubExpr[i]; - osExpr += ")"; + AddSubExpr(i); } osExpr += ")"; break; case SWQ_BETWEEN: CPLAssert(nSubExprCount == 3); - osExpr.Printf("%s %s (%s) AND (%s)", apszSubExpr[0], poOp->pszName, - apszSubExpr[1], apszSubExpr[2]); + AddSubExpr(0); + osExpr += ' '; + osExpr += poOp->pszName; + osExpr += ' '; + AddSubExpr(1); + osExpr += " AND "; + AddSubExpr(2); break; case SWQ_CAST: @@ -760,7 +764,7 @@ CPLString swq_expr_node::UnparseOperationFromUnparsedSubExpr(char **apszSubExpr) osExpr += apszSubExpr[i] + 1; } else - osExpr += apszSubExpr[i]; + AddSubExpr(i); if (i == 1 && nSubExprCount > 2) osExpr += "("; @@ -779,9 +783,7 @@ CPLString swq_expr_node::UnparseOperationFromUnparsedSubExpr(char **apszSubExpr) { if (i > 0) osExpr += ","; - osExpr += "("; - osExpr += apszSubExpr[i]; - osExpr += ")"; + AddSubExpr(i); } osExpr += ")"; break; From 17bd93433d68009dbb78845b9daff84d45ad5496 Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Fri, 1 Nov 2024 13:37:58 +0100 Subject: [PATCH 2/3] OGR SQL: fix wrong handling of NULL expression in AND and NOT (3.10.0 regression) Fixes https://github.com/ropensci/osmextract/issues/298 (provided they also fix their SQL expressions) --- autotest/ogr/ogr_sql_test.py | 7 +++++++ ogr/swq_op_general.cpp | 7 ++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/autotest/ogr/ogr_sql_test.py b/autotest/ogr/ogr_sql_test.py index bf2581e52d8b..fe517750e5d3 100755 --- a/autotest/ogr/ogr_sql_test.py +++ b/autotest/ogr/ogr_sql_test.py @@ -1952,6 +1952,13 @@ def get_available_dialects(): ("(NOT intfield = 0) AND NOT (intfield IS NULL)", 1), ("NOT (intfield = 0 OR intfield IS NOT NULL)", 0), ("(NOT intfield = 0) AND NOT (intfield IS NOT NULL)", 0), + ("intfield <> 0 AND intfield <> 2", 1), + ("intfield IS NOT NULL AND intfield NOT IN (2)", 1), + ("NOT(intfield NOT IN (1) AND NULL NOT IN (1))", 1), + ("NOT(intfield IS NOT NULL AND intfield NOT IN (2))", 1), + ("NOT(NOT(intfield IS NOT NULL AND intfield NOT IN (2)))", 1), + ("NOT (intfield = 0 AND intfield = 0)", 1), + ("(intfield NOT IN (1) AND NULL NOT IN (1)) IS NULL", 1), # realfield ("1 + realfield >= 0", 1), ("realfield = 0", 0), diff --git a/ogr/swq_op_general.cpp b/ogr/swq_op_general.cpp index 6d85f39df5cc..809161897c78 100644 --- a/ogr/swq_op_general.cpp +++ b/ogr/swq_op_general.cpp @@ -521,6 +521,7 @@ swq_expr_node *SWQGeneralEvaluator(swq_expr_node *node, poRet->field_type = node->field_type; if (node->nOperation != SWQ_ISNULL && node->nOperation != SWQ_OR && + node->nOperation != SWQ_AND && node->nOperation != SWQ_NOT && node->nOperation != SWQ_IN) { for (int i = 0; i < node->nSubExprCount; i++) @@ -543,6 +544,8 @@ swq_expr_node *SWQGeneralEvaluator(swq_expr_node *node, case SWQ_AND: poRet->int_value = sub_node_values[0]->int_value && sub_node_values[1]->int_value; + poRet->is_null = + sub_node_values[0]->is_null && sub_node_values[1]->is_null; break; case SWQ_OR: @@ -553,7 +556,9 @@ swq_expr_node *SWQGeneralEvaluator(swq_expr_node *node, break; case SWQ_NOT: - poRet->int_value = !sub_node_values[0]->int_value; + poRet->int_value = !sub_node_values[0]->int_value && + !sub_node_values[0]->is_null; + poRet->is_null = sub_node_values[0]->is_null; break; case SWQ_EQ: From f7836d163b483272c31acd4f616b10b6f47f69d2 Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Fri, 1 Nov 2024 13:43:37 +0100 Subject: [PATCH 3/3] MIGRATION_GUIDE.TXT: mention change w.r.t OGR SQL and NULL values --- MIGRATION_GUIDE.TXT | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/MIGRATION_GUIDE.TXT b/MIGRATION_GUIDE.TXT index 1e3f841f6aa4..6cc284dd316b 100644 --- a/MIGRATION_GUIDE.TXT +++ b/MIGRATION_GUIDE.TXT @@ -1,6 +1,14 @@ MIGRATION GUIDE FROM GDAL 3.9 to GDAL 3.10 ------------------------------------------ +- The OGR SQL parser has been modified to evaluate NULL values in boolean + operations similarly to other SQL engines (SQLite, PostgreSQL, etc.). Previously, + with a foo=NULL field, expressions ``foo NOT IN ('bar')`` and ``foo NOT LIKE ('bar')`` + would evaluate as true. Now the result is false (with the NULL state being + propagated to it). Concretely, to get the same results as in previous versions, + the above expressions must be rewritten as ``foo IS NULL OR foo NOT IN ('bar')`` + and ``foo IS NULL OR foo NOT LIKE ('bar')``. + - MEM driver: opening a dataset with the MEM::: syntax is now disabled by default because of security implications. This can be enabled by setting the GDAL_MEM_ENABLE_OPEN build or configuration option. Creation of a 0-band MEM