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

Tables: Query support for dictionary entities and CreateFilter #12366

Merged
merged 6 commits into from
Jun 1, 2020
Merged

Tables: Query support for dictionary entities and CreateFilter #12366

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f3d0a2d
Support for dictionary entities and CreateFilter
christothes May 29, 2020
c944e90
add comment to CreateFilter
christothes May 29, 2020
819280c
Merge remote-tracking branch 'upstream/master' into users/chriss/quer…
christothes Jun 1, 2020
fe112d7
take the new Azure.ClientSdk.Analyzers and remove pragmas
christothes Jun 1, 2020
98cd3b1
remove redundant .ToList() calls in tests
christothes Jun 1, 2020
0945050
make CreateFilter less prominant on the client
christothes Jun 1, 2020
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
11 changes: 7 additions & 4 deletions sdk/tables/Azure.Data.Tables/api/Azure.Data.Tables.netstandard2.0.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ public partial class TableClient
protected TableClient() { }
public virtual Azure.Response<Azure.Data.Tables.Models.TableItem> Create(System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public virtual System.Threading.Tasks.Task<Azure.Response<Azure.Data.Tables.Models.TableItem>> CreateAsync(System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public string CreateFilter<T>(System.Linq.Expressions.Expression<System.Func<T, bool>> filter) { throw null; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Create Filter sounds a little bit like it's a service operation.

Who would consume this method?

Copy link
Member Author

Choose a reason for hiding this comment

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

The feature crew had a discussion last week about query functionality and the idea was raised to provide a mechanism for developers to easily see the filter query parsed out of their filter expression.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, is it to support mixed query generation scenarios where you generate part of the query and manually format the other part?

Copy link
Member Author

Choose a reason for hiding this comment

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

That could be one, but there were a couple other primary concerns. Firstly, for transparency it would be nice to make it easy for developers to inspect the filter that they generate from the expression. Secondly, this makes it possible to generate and cache a query for later use.

Copy link
Contributor

Choose a reason for hiding this comment

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

Firstly, for transparency it would be nice to make it easy for developers to inspect the filter that they generate from the expression

Do you think our typical HTTP level logging is not enough in this case?

Secondly, this makes it possible to generate and cache a query for later use.

Is performance around query generation a know issue that customers hit?

I'm not strictly against exposing this feature just wonder if doing it proactively is worth it and if the client type is the right place for it (as opposed to a less visible static method somewhere).

Copy link
Member Author

Choose a reason for hiding this comment

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

That's fair - Let me try it with an extension method on TableClient

public virtual Azure.Response Delete(string partitionKey, string rowKey, string eTag = "*", System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public virtual System.Threading.Tasks.Task<Azure.Response> DeleteAsync(string partitionKey, string rowKey, string eTag = "*", System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public virtual Azure.Response<System.Collections.ObjectModel.ReadOnlyCollection<Azure.Data.Tables.Models.SignedIdentifier>> GetAccessPolicy(int? timeout = default(int?), string requestId = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
Expand All @@ -17,11 +18,13 @@ protected TableClient() { }
public virtual Azure.Response<T> Insert<T>(T entity, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) where T : Azure.Data.Tables.TableEntity, new() { throw null; }
public virtual Azure.Response Merge(System.Collections.Generic.IDictionary<string, object> entity, string eTag = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public virtual System.Threading.Tasks.Task<Azure.Response> MergeAsync(System.Collections.Generic.IDictionary<string, object> entity, string eTag = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public virtual Azure.Pageable<System.Collections.Generic.IDictionary<string, object>> Query(string select = null, string filter = null, int? top = default(int?), System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public virtual Azure.AsyncPageable<System.Collections.Generic.IDictionary<string, object>> QueryAsync(string select = null, string filter = null, int? top = default(int?), System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public virtual Azure.AsyncPageable<T> QueryAsync<T>(System.Linq.Expressions.Expression<System.Func<T, bool>> filter, string select = null, int? top = default(int?), System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) where T : Azure.Data.Tables.TableEntity, new() { throw null; }
public virtual Azure.Pageable<System.Collections.Generic.IDictionary<string, object>> Query(System.Linq.Expressions.Expression<System.Func<System.Collections.Generic.IDictionary<string, object>, bool>> filter, int? top = default(int?), string select = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public virtual Azure.Pageable<System.Collections.Generic.IDictionary<string, object>> Query(string filter = null, int? top = default(int?), string select = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public virtual Azure.AsyncPageable<System.Collections.Generic.IDictionary<string, object>> QueryAsync(System.Linq.Expressions.Expression<System.Func<System.Collections.Generic.IDictionary<string, object>, bool>> filter, int? top = default(int?), string select = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public virtual Azure.AsyncPageable<System.Collections.Generic.IDictionary<string, object>> QueryAsync(string filter = null, int? top = default(int?), string select = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public virtual Azure.AsyncPageable<T> QueryAsync<T>(System.Linq.Expressions.Expression<System.Func<T, bool>> filter, int? top = default(int?), string select = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) where T : Azure.Data.Tables.TableEntity, new() { throw null; }
public virtual Azure.AsyncPageable<T> QueryAsync<T>(string filter = null, int? top = default(int?), string select = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) where T : Azure.Data.Tables.TableEntity, new() { throw null; }
public virtual Azure.Pageable<T> Query<T>(System.Linq.Expressions.Expression<System.Func<T, bool>> filter, string select = null, int? top = default(int?), System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) where T : Azure.Data.Tables.TableEntity, new() { throw null; }
public virtual Azure.Pageable<T> Query<T>(System.Linq.Expressions.Expression<System.Func<T, bool>> filter, int? top = default(int?), string select = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) where T : Azure.Data.Tables.TableEntity, new() { throw null; }
public virtual Azure.Pageable<T> Query<T>(string filter = null, int? top = default(int?), string select = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) where T : Azure.Data.Tables.TableEntity, new() { throw null; }
public virtual Azure.Response SetAccessPolicy(System.Collections.Generic.IEnumerable<Azure.Data.Tables.Models.SignedIdentifier> tableAcl, int? timeout = default(int?), string requestId = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public virtual System.Threading.Tasks.Task<Azure.Response> SetAccessPolicyAsync(System.Collections.Generic.IEnumerable<Azure.Data.Tables.Models.SignedIdentifier> tableAcl = null, int? timeout = default(int?), string requestId = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
Expand Down
29 changes: 21 additions & 8 deletions sdk/tables/Azure.Data.Tables/src/Queryable/ExpressionNormalizer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,22 @@ internal override Expression VisitBinary(BinaryExpression b)
{
BinaryExpression visited = (BinaryExpression)base.VisitBinary(b);

if (visited.NodeType == ExpressionType.Equal)
switch (visited.NodeType)
{
Expression normalizedLeft = UnwrapObjectConvert(visited.Left);
Expression normalizedRight = UnwrapObjectConvert(visited.Right);
if (normalizedLeft != visited.Left || normalizedRight != visited.Right)
{
visited = CreateRelationalOperator(ExpressionType.Equal, normalizedLeft, normalizedRight);
}
case ExpressionType.Equal:
case ExpressionType.NotEqual:
case ExpressionType.LessThan:
case ExpressionType.LessThanOrEqual:
case ExpressionType.GreaterThan:
case ExpressionType.GreaterThanOrEqual:

Expression normalizedLeft = UnwrapObjectConvert(visited.Left);
Expression normalizedRight = UnwrapObjectConvert(visited.Right);
if (normalizedLeft != visited.Left || normalizedRight != visited.Right)
{
visited = CreateRelationalOperator(visited.NodeType, normalizedLeft, normalizedRight);
}
break;
}

if (_patterns.TryGetValue(visited.Left, out Pattern pattern) && pattern.Kind == PatternKind.Compare && IsConstantZero(visited.Right))
Expand Down Expand Up @@ -84,7 +92,7 @@ private static Expression UnwrapObjectConvert(Expression input)
}
}

while (ExpressionType.Convert == input.NodeType && typeof(object) == input.Type)
while (ExpressionType.Convert == input.NodeType)
{
input = ((UnaryExpression)input).Operand;
}
Expand Down Expand Up @@ -134,6 +142,11 @@ internal Expression VisitMethodCallNoRewrite(MethodCallExpression call)
return CreateCompareExpression(visited.Arguments[0], visited.Arguments[1]);
}

if (visited.Method == ReflectionUtil.DictionaryGetItemMethodInfo && visited.Arguments.Count == 1 && visited.Arguments[0] is ConstantExpression ce)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at ExpressionNormalizer and ExpressionWriter I wonder if we really need both or if they can be merged into one with additional benefit of avoiding creating of more Expression nodes just to throw them away.

Copy link
Member Author

Choose a reason for hiding this comment

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

In some cases the normalizer is simplifying the expression to be written by the writer. I'd like to do an optimization pass at some point to address issues like this and others.

{
return visited;
}

throw new NotSupportedException($"Method {visited.Method.Name} not supported.");
}

Expand Down
27 changes: 24 additions & 3 deletions sdk/tables/Azure.Data.Tables/src/Queryable/ExpressionWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,19 @@ internal override Expression Visit(Expression exp)
return result;
}

internal override Expression VisitMethodCall(MethodCallExpression m)
{
if (m.Method == ReflectionUtil.DictionaryGetItemMethodInfo && m.Arguments.Count == 1 && m.Arguments[0] is ConstantExpression ce)
{
_builder.Append(ce.Value as string);
}
else
{
return base.VisitMethodCall(m);
}

return m;
}

internal override Expression VisitMemberAccess(MemberExpression m)
{
Expand Down Expand Up @@ -155,9 +168,17 @@ private void VisitOperand(Expression e)
{
if (e is BinaryExpression || e is UnaryExpression)
{
_builder.Append(UriHelper.LEFTPAREN);
Visit(e);
_builder.Append(UriHelper.RIGHTPAREN);
if (e is UnaryExpression unary && unary.NodeType == ExpressionType.TypeAs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a functional benefit in dropping the parenthesis for casts?

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case the parens are not useful. The cast is only needed for the comparison to satisfy the compiler in that all dictionary entries are object so the writer is basically ignoring it. Leaving the parens here results in something like:
(PartitionKey) eq "foo"

{
Visit(unary.Operand);
}
else
{
_builder.Append(UriHelper.LEFTPAREN);
Visit(e);
_builder.Append(UriHelper.RIGHTPAREN);
}

}
else
{
Expand Down
18 changes: 18 additions & 0 deletions sdk/tables/Azure.Data.Tables/src/Queryable/ReflectionUtil.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System.Collections.Generic;
using System.Reflection;

namespace Azure.Data.Tables.Queryable
{
internal static class ReflectionUtil
{
internal static MethodInfo DictionaryGetItemMethodInfo { get; }

static ReflectionUtil()
{
DictionaryGetItemMethodInfo = typeof(IDictionary<string, object>).GetMethod("get_Item");
}
}
}
41 changes: 33 additions & 8 deletions sdk/tables/Azure.Data.Tables/src/TableClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,13 +408,13 @@ public virtual Response Merge(IDictionary<string, object> entity, string eTag =
/// <summary>
/// Queries entities in the table.
/// </summary>
/// <param name="select">Returns the desired properties of an entity from the set. </param>
/// <param name="filter">Returns only tables or entities that satisfy the specified filter.</param>
/// <param name="top">Returns only the top n tables or entities from the set.</param>
/// <param name="select">Returns the desired properties of an entity from the set. </param>
/// <param name="cancellationToken">A <see cref="CancellationToken"/> controlling the request lifetime.</param>
/// <returns></returns>
[ForwardsClientCalls]
public virtual AsyncPageable<IDictionary<string, object>> QueryAsync(string select = null, string filter = null, int? top = null, CancellationToken cancellationToken = default)
public virtual AsyncPageable<IDictionary<string, object>> QueryAsync(string filter = null, int? top = null, string select = null, CancellationToken cancellationToken = default)
{
return PageableHelpers.CreateAsyncEnumerable(async _ =>
{
Expand Down Expand Up @@ -450,13 +450,13 @@ public virtual AsyncPageable<IDictionary<string, object>> QueryAsync(string sele
/// <summary>
/// Queries entities in the table.
/// </summary>
/// <param name="select">Returns the desired properties of an entity from the set. </param>
/// <param name="filter">Returns only tables or entities that satisfy the specified filter.</param>
/// <param name="top">Returns only the top n tables or entities from the set.</param>
/// <param name="select">Returns the desired properties of an entity from the set. </param>
/// <param name="cancellationToken">A <see cref="CancellationToken"/> controlling the request lifetime.</param>

[ForwardsClientCalls]
public virtual Pageable<IDictionary<string, object>> Query(string select = null, string filter = null, int? top = null, CancellationToken cancellationToken = default)
public virtual Pageable<IDictionary<string, object>> Query(string filter = null, int? top = null, string select = null, CancellationToken cancellationToken = default)
{
return PageableHelpers.CreateEnumerable(_ =>
{
Expand Down Expand Up @@ -493,13 +493,30 @@ public virtual Pageable<IDictionary<string, object>> Query(string select = null,
/// Queries entities in the table.
/// </summary>
/// <param name="filter">Returns only tables or entities that satisfy the specified filter.</param>
/// <param name="top">Returns only the top n tables or entities from the set.</param>
/// <param name="select">Returns the desired properties of an entity from the set. </param>
/// <param name="cancellationToken">A <see cref="CancellationToken"/> controlling the request lifetime.</param>

[ForwardsClientCalls]
#pragma warning disable AZC0004 // DO provide both asynchronous and synchronous variants for all service methods.
public virtual AsyncPageable<IDictionary<string, object>> QueryAsync(Expression<Func<IDictionary<string, object>, bool>> filter, int? top = null, string select = null, CancellationToken cancellationToken = default) =>
#pragma warning restore AZC0004 // DO provide both asynchronous and synchronous variants for all service methods.
QueryAsync(Bind(filter), top, select, cancellationToken);

public virtual Pageable<IDictionary<string, object>> Query(Expression<Func<IDictionary<string, object>, bool>> filter, int? top = null, string select = null, CancellationToken cancellationToken = default) =>
Query(Bind(filter), top, select, cancellationToken);

/// <summary>
/// Queries entities in the table.
/// </summary>
/// <param name="filter">Returns only tables or entities that satisfy the specified filter.</param>
/// <param name="top">Returns only the top n tables or entities from the set.</param>
/// <param name="select">Returns the desired properties of an entity from the set. </param>
/// <param name="cancellationToken">A <see cref="CancellationToken"/> controlling the request lifetime.</param>

[ForwardsClientCalls]
#pragma warning disable AZC0004 // DO provide both asynchronous and synchronous variants for all service methods.
public virtual AsyncPageable<T> QueryAsync<T>(Expression<Func<T, bool>> filter, string select = null, int? top = null, CancellationToken cancellationToken = default) where T : TableEntity, new() =>
public virtual AsyncPageable<T> QueryAsync<T>(Expression<Func<T, bool>> filter, int? top = null, string select = null, CancellationToken cancellationToken = default) where T : TableEntity, new() =>
#pragma warning restore AZC0004 // DO provide both asynchronous and synchronous variants for all service methods.
QueryAsync<T>(Bind(filter), top, select, cancellationToken);

Expand Down Expand Up @@ -545,12 +562,12 @@ public virtual Pageable<IDictionary<string, object>> Query(string select = null,
/// Queries entities in the table.
/// </summary>
/// <param name="filter">Returns only tables or entities that satisfy the specified filter.</param>
/// <param name="select">Returns the desired properties of an entity from the set. </param>
/// <param name="top">Returns only the top n tables or entities from the set.</param>
/// <param name="select">Returns the desired properties of an entity from the set. </param>
/// <param name="cancellationToken">A <see cref="CancellationToken"/> controlling the request lifetime.</param>

[ForwardsClientCalls]
public virtual Pageable<T> Query<T>(Expression<Func<T, bool>> filter, string select = null, int? top = null, CancellationToken cancellationToken = default) where T : TableEntity, new() =>
public virtual Pageable<T> Query<T>(Expression<Func<T, bool>> filter, int? top = null, string select = null, CancellationToken cancellationToken = default) where T : TableEntity, new() =>
Query<T>(Bind(filter), top, select, cancellationToken);

/// <summary>
Expand Down Expand Up @@ -675,6 +692,14 @@ public virtual async Task<Response> SetAccessPolicyAsync(IEnumerable<SignedIdent
public virtual Response SetAccessPolicy(IEnumerable<SignedIdentifier> tableAcl, int? timeout = null, string requestId = null, CancellationToken cancellationToken = default) =>
_tableOperations.SetAccessPolicy(_table, timeout, requestId, tableAcl, cancellationToken);

/// <summary>
/// Creates an Odata filter query string from the provided expression.
/// </summary>
/// <typeparam name="T">The type of the entity being queried. Typically this will be derrived from <see cref="TableEntity"/> or <see cref="Dictionary{String, Object}"/>.</typeparam>
/// <param name="filter">A filter expresssion.</param>
/// <returns>The string representation of the filter expression.</returns>
public string CreateFilter<T>(Expression<Func<T, bool>> filter) => Bind(filter);

internal ExpressionParser GetExpressionParser()
{
if (_isPremiumEndpoint)
Expand All @@ -688,7 +713,7 @@ internal ExpressionParser GetExpressionParser()
}
}

internal string Bind(Expression expression)
private string Bind(Expression expression)
{
Argument.AssertNotNull(expression, nameof(expression));

Expand Down
Loading