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.

Sign up for GitHub

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

[Backport release/3.10] OGR SQL: fix wrong handling of NULL expression in AND and NOT (3.10.0 regression) #11191

Merged
merged 3 commits into from
Nov 1, 2024
Merged

[Backport release/3.10] OGR SQL: fix wrong handling of NULL expression in AND and NOT (3.10.0 regression) #11191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2230456
SQL expression tree to string: add missing parenthesis that could mak…
rouault Nov 1, 2024
677617c
OGR SQL: fix wrong handling of NULL expression in AND and NOT (3.10.0…
rouault Nov 1, 2024
10473e0
MIGRATION_GUIDE.TXT: mention change w.r.t OGR SQL and NULL values
rouault Nov 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions MIGRATION_GUIDE.TXT
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down
9 changes: 4 additions & 5 deletions autotest/cpp/test_ogr_swq.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
};
}
Expand Down
7 changes: 7 additions & 0 deletions autotest/ogr/ogr_sql_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
72 changes: 37 additions & 35 deletions ogr/swq_expr_node.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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:
Expand All @@ -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 += "(";
Expand All @@ -779,9 +783,7 @@ CPLString swq_expr_node::UnparseOperationFromUnparsedSubExpr(char **apszSubExpr)
{
if (i > 0)
osExpr += ",";
osExpr += "(";
osExpr += apszSubExpr[i];
osExpr += ")";
AddSubExpr(i);
}
osExpr += ")";
break;
Expand Down
7 changes: 6 additions & 1 deletion ogr/swq_op_general.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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++)
Expand All @@ -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:
Expand All @@ -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:
Expand Down
Loading