You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2020/04/06 14:12:19 UTC
[GitHub] [ignite] gurustron opened a new pull request #7628: IGNITE-7609
.NET: FieldsQueryCursor should expose data types too
gurustron opened a new pull request #7628: IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
URL: https://github.com/apache/ignite/pull/7628
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] gurustron commented on a change in pull request #7628:
IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
Posted by GitBox <gi...@apache.org>.
gurustron commented on a change in pull request #7628: IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
URL: https://github.com/apache/ignite/pull/7628#discussion_r405776267
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Cache/Query/FieldsQueryCursor.cs
##########
@@ -85,5 +92,26 @@ public IList<string> FieldNames
Target.OutStream(OpGetFieldNames, reader => reader.ReadStringCollection())));
}
}
+
+ /** <inheritdoc /> */
+ public IList<IQueryCursorField> Fields
+ {
+ get
+ {
+ if (_fieldsMeta == null)
+ {
+ var metadata = Target.OutStream(
+ OpGetFieldsMeta,
+ reader => reader.ReadCollectionRaw(stream =>
+ new QueryCursorField(stream) as IQueryCursorField))
+ ?? new List<IQueryCursorField>();
+
+ _fieldsMeta = new ReadOnlyCollection<IQueryCursorField>(
+ metadata as List<IQueryCursorField> ?? metadata.ToList());
Review comment:
Changed the underlying method to return IList since it is already returning one
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] ptupitsyn merged pull request #7628: IGNITE-7609 .NET:
FieldsQueryCursor should expose data types too
Posted by GitBox <gi...@apache.org>.
ptupitsyn merged pull request #7628: IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
URL: https://github.com/apache/ignite/pull/7628
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] ptupitsyn commented on a change in pull request #7628:
IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #7628: IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
URL: https://github.com/apache/ignite/pull/7628#discussion_r404138616
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/platform/cache/query/PlatformFieldsQueryCursor.java
##########
@@ -70,6 +84,17 @@ public PlatformFieldsQueryCursor(PlatformContext platformCtx, QueryCursorEx<List
for (int i = 0; i < cnt; i++) {
writer.writeString(fq.getFieldName(i));
}
+ } else if (type == OP_GET_FIELDS_META) {
+ QueryCursorEx<List<?>> cursor = cursor();
+
+ List<GridQueryFieldMetadata> metadatas = cursor.fieldsMeta();
Review comment:
`metadatas` -> `metas`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] ptupitsyn commented on a change in pull request #7628:
IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #7628: IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
URL: https://github.com/apache/ignite/pull/7628#discussion_r405688871
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Cache/Query/QueryCursorField.cs
##########
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+namespace Apache.Ignite.Core.Impl.Cache.Query
+{
+ using System;
+ using Apache.Ignite.Core.Binary;
+ using Apache.Ignite.Core.Cache.Query;
+ using Apache.Ignite.Core.Impl.Binary;
+
+ /// <summary>
+ /// Query cursor field implementation.
+ /// </summary>
+ internal class QueryCursorField : IQueryCursorField
+ {
+ /// <summary>
+ /// Initializes a new instance of the <see cref="QueryCursorField"/> class.
+ /// </summary>
+ /// <param name="reader">The reader.</param>
+ public QueryCursorField(IBinaryRawReader reader)
+ {
+ Name = reader.ReadString();
+ JavaTypeName = reader.ReadString();
+ Type = GetDotNetType(JavaTypeName);
+ }
+
+ /** <inheritdoc /> */
+ public string Name { get; private set; }
+
+ /** <inheritdoc /> */
+ public string JavaTypeName { get; private set; }
+
+ /** <inheritdoc /> */
+ public Type Type { get; private set; }
+
+ /// <summary>
+ /// Gets .NET type that corresponds to specified Java type name.
+ /// </summary>
+ /// <param name="javaTypeName">Name of the java type.</param>
+ /// <returns></returns>
+ private static Type GetDotNetType(string javaTypeName)
+ {
+ var dotNetType = JavaTypes.GetDotNetType(javaTypeName);
+ if (dotNetType == null && javaTypeName == "java.lang.Object") dotNetType = typeof(object);
Review comment:
Can we move this check to `JavaTypes` class?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] ptupitsyn commented on a change in pull request #7628:
IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #7628: IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
URL: https://github.com/apache/ignite/pull/7628#discussion_r404139127
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/platform/cache/query/PlatformFieldsQueryCursor.java
##########
@@ -70,6 +84,17 @@ public PlatformFieldsQueryCursor(PlatformContext platformCtx, QueryCursorEx<List
for (int i = 0; i < cnt; i++) {
writer.writeString(fq.getFieldName(i));
}
+ } else if (type == OP_GET_FIELDS_META) {
+ QueryCursorEx<List<?>> cursor = cursor();
+
+ List<GridQueryFieldMetadata> metadatas = cursor.fieldsMeta();
+
+ writer.writeInt(metadatas.size());
Review comment:
`cursor.fieldsMeta()` is not guaranteed to return a non-null result, let's add a check.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] ptupitsyn commented on a change in pull request #7628:
IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #7628: IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
URL: https://github.com/apache/ignite/pull/7628#discussion_r405682407
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/platform/cache/query/PlatformFieldsQueryCursor.java
##########
@@ -22,29 +22,40 @@
import org.apache.ignite.internal.binary.BinaryRawWriterEx;
import org.apache.ignite.internal.processors.cache.query.QueryCursorEx;
import org.apache.ignite.internal.processors.platform.PlatformContext;
+import org.apache.ignite.internal.processors.query.GridQueryFieldMetadata;
import java.util.List;
/**
* Interop cursor for fields query.
*/
public class PlatformFieldsQueryCursor extends PlatformAbstractQueryCursor<List<?>> {
- /** Gets field names. */
+ /**
Review comment:
Something happened with comments blocks here and below. Please revert unrelated changes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] ptupitsyn commented on a change in pull request #7628:
IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #7628: IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
URL: https://github.com/apache/ignite/pull/7628#discussion_r404153038
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Cache/Query/QueryCursorFieldMetadataImpl.cs
##########
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+namespace Apache.Ignite.Core.Impl.Cache.Query
+{
+ using System;
+ using Apache.Ignite.Core.Binary;
+ using Apache.Ignite.Core.Cache.Query;
+ using Apache.Ignite.Core.Impl.Binary;
+
+ /// <summary>
+ /// Query cursor field metadata implementation.
+ /// </summary>
+ public class QueryCursorFieldMetadataImpl : IQueryCursorFieldMetadata
Review comment:
**Important**: make this class `internal`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] ptupitsyn commented on a change in pull request #7628:
IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #7628: IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
URL: https://github.com/apache/ignite/pull/7628#discussion_r404151673
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Cache/Query/IQueryCursorFieldMetadata.cs
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+namespace Apache.Ignite.Core.Cache.Query
+{
+ using System;
+
+ /// <summary>
+ /// Query field descriptor. This descriptor is used to provide metadata
+ /// about fields returned in query result.
+ /// </summary>
+ public interface IQueryCursorFieldMetadata
+ {
+ /// <summary>
+ /// Field name
+ /// </summary>
+ string Name { get; }
+
+ /// <summary>
+ /// Name of java type to which this field belongs
+ /// </summary>
+ string JavaTypeName { get; }
+
+ /// <summary>
+ /// Type to which this field belongs
Review comment:
`Gets the field type`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] gurustron commented on a change in pull request #7628:
IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
Posted by GitBox <gi...@apache.org>.
gurustron commented on a change in pull request #7628: IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
URL: https://github.com/apache/ignite/pull/7628#discussion_r405776537
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/platform/cache/query/PlatformFieldsQueryCursor.java
##########
@@ -22,29 +22,40 @@
import org.apache.ignite.internal.binary.BinaryRawWriterEx;
import org.apache.ignite.internal.processors.cache.query.QueryCursorEx;
import org.apache.ignite.internal.processors.platform.PlatformContext;
+import org.apache.ignite.internal.processors.query.GridQueryFieldMetadata;
import java.util.List;
/**
* Interop cursor for fields query.
*/
public class PlatformFieldsQueryCursor extends PlatformAbstractQueryCursor<List<?>> {
- /** Gets field names. */
+ /**
Review comment:
Auto-formatting by IDE.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] ptupitsyn commented on a change in pull request #7628:
IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #7628: IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
URL: https://github.com/apache/ignite/pull/7628#discussion_r405683139
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Cache/Query/IFieldsQueryCursor.cs
##########
@@ -19,6 +19,7 @@ namespace Apache.Ignite.Core.Cache.Query
{
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
+ using Apache.Ignite.Core.Impl.Cache.Query;
Review comment:
Unused import?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] ptupitsyn commented on a change in pull request #7628:
IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #7628: IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
URL: https://github.com/apache/ignite/pull/7628#discussion_r404141432
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Cache/Query/IFieldsQueryCursor.cs
##########
@@ -30,5 +31,10 @@ public interface IFieldsQueryCursor : IQueryCursor<IList<object>>
/// Gets the field names.
/// </summary>
IList<string> FieldNames { get; }
+
+ /// <summary>
+ /// Gets fields metadata
Review comment:
Comment should end with `.`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] ptupitsyn commented on a change in pull request #7628:
IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #7628: IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
URL: https://github.com/apache/ignite/pull/7628#discussion_r404145832
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Cache/Query/FieldsQueryCursor.cs
##########
@@ -85,5 +92,24 @@ public IList<string> FieldNames
Target.OutStream(OpGetFieldNames, reader => reader.ReadStringCollection())));
}
}
+
+ public IList<IQueryCursorFieldMetadata> FieldsMetadata
Review comment:
Missing XMLDoc
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] ptupitsyn commented on a change in pull request #7628:
IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #7628: IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
URL: https://github.com/apache/ignite/pull/7628#discussion_r404140315
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/platform/cache/query/PlatformFieldsQueryCursor.java
##########
@@ -70,6 +84,17 @@ public PlatformFieldsQueryCursor(PlatformContext platformCtx, QueryCursorEx<List
for (int i = 0; i < cnt; i++) {
writer.writeString(fq.getFieldName(i));
}
+ } else if (type == OP_GET_FIELDS_META) {
+ QueryCursorEx<List<?>> cursor = cursor();
+
+ List<GridQueryFieldMetadata> metadatas = cursor.fieldsMeta();
Review comment:
https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules
Weird rules, but we follow them.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] ptupitsyn commented on a change in pull request #7628:
IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #7628: IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
URL: https://github.com/apache/ignite/pull/7628#discussion_r404148087
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Cache/Query/IFieldsQueryCursor.cs
##########
@@ -30,5 +31,10 @@ public interface IFieldsQueryCursor : IQueryCursor<IList<object>>
/// Gets the field names.
/// </summary>
IList<string> FieldNames { get; }
+
+ /// <summary>
+ /// Gets fields metadata
+ /// </summary>
+ IList<IQueryCursorFieldMetadata> FieldsMetadata { get; }
Review comment:
Let's just name this `Fields`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] ptupitsyn commented on a change in pull request #7628:
IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #7628: IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
URL: https://github.com/apache/ignite/pull/7628#discussion_r404151524
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Cache/Query/IQueryCursorFieldMetadata.cs
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+namespace Apache.Ignite.Core.Cache.Query
+{
+ using System;
+
+ /// <summary>
+ /// Query field descriptor. This descriptor is used to provide metadata
+ /// about fields returned in query result.
+ /// </summary>
+ public interface IQueryCursorFieldMetadata
+ {
+ /// <summary>
+ /// Field name
+ /// </summary>
+ string Name { get; }
+
+ /// <summary>
+ /// Name of java type to which this field belongs
Review comment:
`Gets the name of Java type for this field`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] ptupitsyn commented on a change in pull request #7628:
IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #7628: IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
URL: https://github.com/apache/ignite/pull/7628#discussion_r404144313
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Cache/Query/FieldsQueryCursor.cs
##########
@@ -33,7 +34,7 @@ internal class FieldsQueryCursor<T> : PlatformQueryQursorBase<T>
/// Constructor.
/// </summary>
/// <param name="target">Target.</param>
- /// <param name="keepBinary">Keep poratble flag.</param>
+ /// <param name="keepBinary">Keep portable flag.</param>
Review comment:
`portable` -> `binary`.
P.S. This is an ancient legacy of early Ignite days. `Binary` was called `Portable`, then we renamed, but this one line was skipped due to a typo.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] gurustron commented on a change in pull request #7628:
IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
Posted by GitBox <gi...@apache.org>.
gurustron commented on a change in pull request #7628: IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
URL: https://github.com/apache/ignite/pull/7628#discussion_r405729244
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Cache/Query/FieldsQueryCursor.cs
##########
@@ -85,5 +92,26 @@ public IList<string> FieldNames
Target.OutStream(OpGetFieldNames, reader => reader.ReadStringCollection())));
}
}
+
+ /** <inheritdoc /> */
+ public IList<IQueryCursorField> Fields
+ {
+ get
+ {
+ if (_fieldsMeta == null)
+ {
+ var metadata = Target.OutStream(
+ OpGetFieldsMeta,
+ reader => reader.ReadCollectionRaw(stream =>
+ new QueryCursorField(stream) as IQueryCursorField))
+ ?? new List<IQueryCursorField>();
+
+ _fieldsMeta = new ReadOnlyCollection<IQueryCursorField>(
+ metadata as List<IQueryCursorField> ?? metadata.ToList());
Review comment:
As far as i understand .AsReadOnly() is not an option here cause we have IList in contract and if the underlying collection is mutable it will allow mutations.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] ptupitsyn commented on a change in pull request #7628:
IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #7628: IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
URL: https://github.com/apache/ignite/pull/7628#discussion_r404151060
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Cache/Query/IQueryCursorFieldMetadata.cs
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+namespace Apache.Ignite.Core.Cache.Query
+{
+ using System;
+
+ /// <summary>
+ /// Query field descriptor. This descriptor is used to provide metadata
+ /// about fields returned in query result.
+ /// </summary>
+ public interface IQueryCursorFieldMetadata
+ {
+ /// <summary>
+ /// Field name
Review comment:
`Gets the field name.`
https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1623.md
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] ptupitsyn commented on a change in pull request #7628:
IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #7628: IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
URL: https://github.com/apache/ignite/pull/7628#discussion_r404152676
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Cache/Query/QueryCursorFieldMetadataImpl.cs
##########
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+namespace Apache.Ignite.Core.Impl.Cache.Query
+{
+ using System;
+ using Apache.Ignite.Core.Binary;
+ using Apache.Ignite.Core.Cache.Query;
+ using Apache.Ignite.Core.Impl.Binary;
+
+ /// <summary>
+ /// Query cursor field metadata implementation.
+ /// </summary>
+ public class QueryCursorFieldMetadataImpl : IQueryCursorFieldMetadata
Review comment:
Please remove `Impl` suffix. It is present on some types because of Java naming legacy, but it is against Microsoft standards, so we try to avoid this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] ptupitsyn commented on a change in pull request #7628:
IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #7628: IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
URL: https://github.com/apache/ignite/pull/7628#discussion_r404139313
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/platform/cache/query/PlatformFieldsQueryCursor.java
##########
@@ -70,6 +84,17 @@ public PlatformFieldsQueryCursor(PlatformContext platformCtx, QueryCursorEx<List
for (int i = 0; i < cnt; i++) {
writer.writeString(fq.getFieldName(i));
}
+ } else if (type == OP_GET_FIELDS_META) {
+ QueryCursorEx<List<?>> cursor = cursor();
+
+ List<GridQueryFieldMetadata> metadatas = cursor.fieldsMeta();
+
+ writer.writeInt(metadatas.size());
+
+ for (GridQueryFieldMetadata metadata: metadatas){
Review comment:
`metadata` -> `meta`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] ptupitsyn commented on a change in pull request #7628:
IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #7628: IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
URL: https://github.com/apache/ignite/pull/7628#discussion_r404148809
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Cache/Query/IQueryCursorFieldMetadata.cs
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+namespace Apache.Ignite.Core.Cache.Query
+{
+ using System;
+
+ /// <summary>
+ /// Query field descriptor. This descriptor is used to provide metadata
+ /// about fields returned in query result.
+ /// </summary>
+ public interface IQueryCursorFieldMetadata
Review comment:
In line with the above: `IQueryCursorField`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [ignite] ptupitsyn commented on a change in pull request #7628:
IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #7628: IGNITE-7609 .NET: FieldsQueryCursor should expose data types too
URL: https://github.com/apache/ignite/pull/7628#discussion_r405687502
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Cache/Query/FieldsQueryCursor.cs
##########
@@ -85,5 +92,26 @@ public IList<string> FieldNames
Target.OutStream(OpGetFieldNames, reader => reader.ReadStringCollection())));
}
}
+
+ /** <inheritdoc /> */
+ public IList<IQueryCursorField> Fields
+ {
+ get
+ {
+ if (_fieldsMeta == null)
+ {
+ var metadata = Target.OutStream(
+ OpGetFieldsMeta,
+ reader => reader.ReadCollectionRaw(stream =>
+ new QueryCursorField(stream) as IQueryCursorField))
+ ?? new List<IQueryCursorField>();
+
+ _fieldsMeta = new ReadOnlyCollection<IQueryCursorField>(
+ metadata as List<IQueryCursorField> ?? metadata.ToList());
Review comment:
`ToList` causes unnecessary allocations here. My proposal:
* Remove `??` above, get the result of `ReadCollectionRaw` as is
* `_fieldsMeta = meta != null ? meta.AsReadOnly() : new ReadOnlyCollection()`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services