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

Use -> operator for SQLite JsonScalarExpression #30745

Merged
merged 1 commit into from
Apr 24, 2023
Merged

Use -> operator for SQLite JsonScalarExpression #30745

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 9 additions & 9 deletions src/EFCore.Relational/Query/QuerySqlGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -560,32 +560,32 @@ protected virtual void CheckComposableSqlTrimmed(ReadOnlySpan<char> sql)
/// <inheritdoc />
protected override Expression VisitSqlBinary(SqlBinaryExpression sqlBinaryExpression)
{
var requiresBrackets = RequiresParentheses(sqlBinaryExpression, sqlBinaryExpression.Left);
var requiresParentheses = RequiresParentheses(sqlBinaryExpression, sqlBinaryExpression.Left);

if (requiresBrackets)
if (requiresParentheses)
{
_relationalCommandBuilder.Append("(");
}

Visit(sqlBinaryExpression.Left);

if (requiresBrackets)
if (requiresParentheses)
{
_relationalCommandBuilder.Append(")");
}

_relationalCommandBuilder.Append(GetOperator(sqlBinaryExpression));

requiresBrackets = RequiresParentheses(sqlBinaryExpression, sqlBinaryExpression.Right);
requiresParentheses = RequiresParentheses(sqlBinaryExpression, sqlBinaryExpression.Right);

if (requiresBrackets)
if (requiresParentheses)
{
_relationalCommandBuilder.Append("(");
}

Visit(sqlBinaryExpression.Right);

if (requiresBrackets)
if (requiresParentheses)
{
_relationalCommandBuilder.Append(")");
}
Expand Down Expand Up @@ -765,14 +765,14 @@ protected override Expression VisitSqlUnary(SqlUnaryExpression sqlUnaryExpressio
case ExpressionType.Convert:
{
_relationalCommandBuilder.Append("CAST(");
var requiresBrackets = RequiresParentheses(sqlUnaryExpression, sqlUnaryExpression.Operand);
if (requiresBrackets)
var requiresParentheses = RequiresParentheses(sqlUnaryExpression, sqlUnaryExpression.Operand);
if (requiresParentheses)
{
_relationalCommandBuilder.Append("(");
}

Visit(sqlUnaryExpression.Operand);
if (requiresBrackets)
if (requiresParentheses)
{
_relationalCommandBuilder.Append(")");
}
Expand Down
2 changes: 1 addition & 1 deletion src/EFCore.Relational/Query/SqlExpressions/JsonScalarExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ internal JsonScalarExpression(
}

/// <summary>
/// The column containg JSON value.
/// The column containing the JSON value.
/// </summary>
public virtual ColumnExpression JsonColumn { get; }

Expand Down
2 changes: 1 addition & 1 deletion src/EFCore.Relational/Query/SqlExpressions/SelectExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1592,7 +1592,7 @@ static Dictionary<JsonQueryExpression, JsonScalarExpression> BuildJsonProjection
{
// force reference comparison for this one, even if we implement custom equality for JsonQueryExpression in the future
var deduplicationMap = new Dictionary<JsonQueryExpression, JsonScalarExpression>(ReferenceEqualityComparer.Instance);
if (projections.Count() > 0)
if (projections.Any())
{
var ordered = projections
.OrderBy(x => $"{x.JsonColumn.TableAlias}.{x.JsonColumn.Name}")
Expand Down
109 changes: 80 additions & 29 deletions src/EFCore.Sqlite.Core/Query/Internal/SqliteQuerySqlGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,56 +126,106 @@ private Expression VisitRegexp(RegexpExpression regexpExpression)
/// </summary>
protected override Expression VisitJsonScalar(JsonScalarExpression jsonScalarExpression)
{
if (jsonScalarExpression.Path.Count == 1
&& jsonScalarExpression.Path[0].ToString() == "$")
// TODO: This trims the leading $ PathSegment, which isn't actually part of the path (but rather path of the JSONPATH language
Copy link
Member Author

@roji roji Apr 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@maumar note this - I think we should remove the $ path segment at the beginning, at the source... It's purely a SQL generation concern, and PostgreSQL would need to ignore it (since it doesn't use JSONPATH for this). Even for SQLite, when there's just a single (non-$) path segment, this PR does JsonColumn ->> 'Foo' (property) or JsonColumn ->> 1 (array), which is simpler and again doesn't need the $.

On a related note, when the JSON expression has no path segments, it shouldn't be created in translation - there's this piece of code here for ignoring/unwrapping it when it's empty.

Does that make sense? If so I can open a separate issue to do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense!

Copy link
Member Author

Choose a reason for hiding this comment

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

#30747

// used to generate the path in SQL for *some* databases).
// Instead, we should not be producing JsonScalarExpression at all with the leading $.
var path = jsonScalarExpression.Path is [{ PropertyName: "$" }, ..]
? jsonScalarExpression.Path.Skip(1).ToArray()
: jsonScalarExpression.Path;

if (path.Count == 0)
{
Visit(jsonScalarExpression.JsonColumn);

return jsonScalarExpression;
}

Sql.Append("json_extract(");

Visit(jsonScalarExpression.JsonColumn);

Sql.Append(", '");
foreach (var pathSegment in jsonScalarExpression.Path)
var inJsonpathString = false;

for (var i = 0; i < path.Count; i++)
{
if (pathSegment.PropertyName != null)
{
Sql.Append((pathSegment.PropertyName == "$" ? "" : ".") + pathSegment.PropertyName);
}
var pathSegment = path[i];
var isLast = i == path.Count - 1;

if (pathSegment.ArrayIndex != null)
switch (pathSegment)
{
Sql.Append("[");
case { PropertyName: string propertyName }:
if (inJsonpathString)
{
Sql.Append(".").Append(propertyName);
continue;
}

if (pathSegment.ArrayIndex is SqlConstantExpression)
{
Visit(pathSegment.ArrayIndex);
}
else
{
Sql.Append("' || ");
if (pathSegment.ArrayIndex is SqlParameterExpression)
Sql.Append(" ->> ");

// No need to start a $. JSONPATH string if we're the last segment or the next segment isn't a constant
if (isLast || path[i + 1] is { ArrayIndex: not null and not SqlConstantExpression })
{
Sql.Append("'").Append(propertyName).Append("'");
continue;
}

Sql.Append("'$.").Append(propertyName);
inJsonpathString = true;
continue;

case { ArrayIndex: SqlConstantExpression arrayIndex }:
if (inJsonpathString)
{
Sql.Append("[");
Visit(pathSegment.ArrayIndex);
Sql.Append("]");
continue;
}
else

Sql.Append(" ->> ");

// No need to start a $. JSONPATH string if we're the last segment or the next segment isn't a constant
if (isLast || path[i + 1] is { ArrayIndex: not null and not SqlConstantExpression })
{
Visit(arrayIndex);
continue;
}

Sql.Append("'$[");
Visit(arrayIndex);
Sql.Append("]");
inJsonpathString = true;
continue;

default:
if (inJsonpathString)
{
Sql.Append("'");
inJsonpathString = false;
}

Sql.Append(" ->> ");

Check.DebugAssert(pathSegment.ArrayIndex is not null, "pathSegment.ArrayIndex is not null");

var requiresParentheses = RequiresParentheses(jsonScalarExpression, pathSegment.ArrayIndex);
if (requiresParentheses)
{
Sql.Append("(");
Visit(pathSegment.ArrayIndex);
Sql.Append(")");
}

Sql.Append(" || '");
}
Visit(pathSegment.ArrayIndex);

if (requiresParentheses)
{
Sql.Append(")");
}

Sql.Append("]");
continue;
}
}

Sql.Append("')");
if (inJsonpathString)
{
Sql.Append("'");
}

return jsonScalarExpression;
}
Expand All @@ -196,7 +246,7 @@ protected override bool TryGetOperatorInfo(SqlExpression expression, out int pre
ExpressionType.Multiply => (900, true),
ExpressionType.Divide => (900, false),
ExpressionType.Modulo => (900, false),
ExpressionType.Add when sqlBinaryExpression.Type == typeof(string) => (1100, true),
ExpressionType.Add when sqlBinaryExpression.Type == typeof(string) => (1000, true),
ExpressionType.Add when sqlBinaryExpression.Type != typeof(string) => (800, true),
ExpressionType.Subtract => (800, false),
ExpressionType.And => (600, true),
Expand Down Expand Up @@ -227,6 +277,7 @@ protected override bool TryGetOperatorInfo(SqlExpression expression, out int pre

CollateExpression => (1100, false),
LikeExpression => (500, false),
JsonScalarExpression => (1000, true),

_ => default,
};
Expand Down
43 changes: 40 additions & 3 deletions test/EFCore.Sqlite.FunctionalTests/Query/JsonQuerySqliteTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,29 @@ public JsonQuerySqliteTest(JsonQuerySqliteFixture fixture, ITestOutputHelper tes
//Fixture.TestSqlLoggerFactory.SetTestOutputHelper(testOutputHelper);
}

public override async Task Json_scalar_length(bool async)
{
await base.Json_scalar_length(async);

AssertSql(
"""
SELECT "j"."Name"
FROM "JsonEntitiesBasic" AS "j"
WHERE length("j"."OwnedReferenceRoot" ->> 'Name') > 2
""");
}

public override async Task Basic_json_projection_enum_inside_json_entity(bool async)
{
await base.Basic_json_projection_enum_inside_json_entity(async);

AssertSql(
"""
SELECT "j"."Id", "j"."OwnedReferenceRoot" ->> '$.OwnedReferenceBranch.Enum' AS "Enum"
FROM "JsonEntitiesBasic" AS "j"
""");
}

public override async Task Project_json_entity_FirstOrDefault_subquery(bool async)
=> Assert.Equal(
SqliteStrings.ApplyNotSupported,
Expand Down Expand Up @@ -67,6 +90,20 @@ await AssertQuery(
""");
}

public override async Task Json_collection_element_access_in_predicate_nested_mix(bool async)
{
await base.Json_collection_element_access_in_predicate_nested_mix(async);

AssertSql(
"""
@__prm_0='0'

SELECT "j"."Id", "j"."EntityBasicId", "j"."Name", "j"."OwnedCollectionRoot", "j"."OwnedReferenceRoot"
FROM "JsonEntitiesBasic" AS "j"
WHERE "j"."OwnedCollectionRoot" ->> '$[1].OwnedCollectionBranch' ->> @__prm_0 ->> 'OwnedCollectionLeaf' ->> ("j"."Id" - 1) ->> 'SomethingSomething' = 'e1_c2_c1_c1'
""");
}

public override async Task Json_predicate_on_bool_converted_to_int_zero_one(bool async)
{
await base.Json_predicate_on_bool_converted_to_int_zero_one(async);
Expand All @@ -75,7 +112,7 @@ public override async Task Json_predicate_on_bool_converted_to_int_zero_one(bool
"""
SELECT "j"."Id", "j"."Reference"
FROM "JsonEntitiesConverters" AS "j"
WHERE json_extract("j"."Reference", '$.BoolConvertedToIntZeroOne') = 1
WHERE "j"."Reference" ->> 'BoolConvertedToIntZeroOne' = 1
""");
}

Expand All @@ -87,7 +124,7 @@ public override async Task Json_predicate_on_bool_converted_to_string_True_False
"""
SELECT "j"."Id", "j"."Reference"
FROM "JsonEntitiesConverters" AS "j"
WHERE json_extract("j"."Reference", '$.BoolConvertedToStringTrueFalse') = 'True'
WHERE "j"."Reference" ->> 'BoolConvertedToStringTrueFalse' = 'True'
""");
}

Expand All @@ -99,7 +136,7 @@ public override async Task Json_predicate_on_bool_converted_to_string_Y_N(bool a
"""
SELECT "j"."Id", "j"."Reference"
FROM "JsonEntitiesConverters" AS "j"
WHERE json_extract("j"."Reference", '$.BoolConvertedToStringYN') = 'Y'
WHERE "j"."Reference" ->> 'BoolConvertedToStringYN' = 'Y'
""");
}

Expand Down