[jira] [Comment Edited] (IGNITE-6055) SQL: Add String length constraint

["Nikolay Izhikov (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/IGNITE-6055?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16522719#comment-16522719 ] 

Nikolay Izhikov edited comment on IGNITE-6055 at 6/25/18 7:34 PM:
------------------------------------------------------------------

[~vozerov]

> 1) org.apache.ignite.internal.processors.query.h2.ddl.DdlStatementsProcessor#toQueryEntity - this check is redundant, we should check whether QueryEntity is valid on cache start, not inside DDL (to handle both DDL and cache create through API in the same place). See org.apache.ignite.internal.processors.query.QueryUtils#validateQueryEntity

Fixed.

> 2) I am not very happy with precision/scale/maxLength properties - too much of them. Instead, we can re-use "precision" for String length. H2 works this way (see their docs), so it should be ok for us as well. In fact, this is why GridSqlColumn.maxLength() return the same value as GridSqlColumn.precision()

Fixed.

> 3) QueryEntity - getters should return current value, without wrapping it into unmodifiable collection, because this is how users frequently use us - QueryEntity.getNotNullFields().add(...)

Fixed.

> 4) Let's split single decimal property into "scale" and "precision". Because single precision would be needed not only for strings, but for other data types as well (e.g. DOUBLE, REAL, BINARY)

Fixed.

> 6) QueryBinaryProperty, QueryTypeDescriptorImpl, QueryUtils - checks for "_KEY" and "_VAL" are illegal, because key/val field names could be overriden with QueryEntity.keyFieldName/valFieldName. These checks should be more generic

Fixed.

> 7) QueryBinaryProperty.value() - new logic around key/val should be removed as it breaks an invariant that only nested fields can be read/written here. Please find a way to perform a check on key/value without changing central field extraction logic.

FIxed.

> 8) We need more tests for cache API. E.g. I do not see tests for invoke().

Tests added.

> 5) I do not see how compatibility is handled in .NET - new fields are serialized unconditionally, meaning that we cannot talk to previous version. Am I wrong?

Do we have compatibility for a .Net client?
I don't see any conditional logic for a "default field value" implementation [1].
Can you give me an example of such logic?

Please, review the PR.

[1] [https://github.com/apache/ignite/commit/78e79e011f1bb017653e628587faf606cfd37510]


was (Author: nizhikov):
> 1) org.apache.ignite.internal.processors.query.h2.ddl.DdlStatementsProcessor#toQueryEntity - this check is redundant, we should check whether QueryEntity is valid on cache start, not inside DDL (to handle both DDL and cache create through API in the same place). See org.apache.ignite.internal.processors.query.QueryUtils#validateQueryEntity

Fixed.

> 2) I am not very happy with precision/scale/maxLength properties - too much of them. Instead, we can re-use "precision" for String length. H2 works this way (see their docs), so it should be ok for us as well. In fact, this is why GridSqlColumn.maxLength() return the same value as GridSqlColumn.precision()

Fixed.

> 3) QueryEntity - getters should return current value, without wrapping it into unmodifiable collection, because this is how users frequently use us - QueryEntity.getNotNullFields().add(...)

Fixed.

> 4) Let's split single decimal property into "scale" and "precision". Because single precision would be needed not only for strings, but for other data types as well (e.g. DOUBLE, REAL, BINARY)

Fixed.

> 6) QueryBinaryProperty, QueryTypeDescriptorImpl, QueryUtils - checks for "_KEY" and "_VAL" are illegal, because key/val field names could be overriden with QueryEntity.keyFieldName/valFieldName. These checks should be more generic

Fixed.

> 7) QueryBinaryProperty.value() - new logic around key/val should be removed as it breaks an invariant that only nested fields can be read/written here. Please find a way to perform a check on key/value without changing central field extraction logic.

FIxed.

> 8) We need more tests for cache API. E.g. I do not see tests for invoke().

Tests added.

> 5) I do not see how compatibility is handled in .NET - new fields are serialized unconditionally, meaning that we cannot talk to previous version. Am I wrong?

Do we have compatibility for a .Net client?
I don't see any conditional logic for a "default field value" implementation [1].
Can you give me an example of such logic?

[1] [https://github.com/apache/ignite/commit/78e79e011f1bb017653e628587faf606cfd37510]

> SQL: Add String length constraint
> ---------------------------------
>
>                 Key: IGNITE-6055
>                 URL: https://issues.apache.org/jira/browse/IGNITE-6055
>             Project: Ignite
>          Issue Type: Task
>          Components: sql
>    Affects Versions: 2.1
>            Reporter: Vladimir Ozerov
>            Assignee: Nikolay Izhikov
>            Priority: Major
>              Labels: sql-engine
>
> We should support {{CHAR(X)}} and {{VARCHAR{X}} syntax. Currently, we ignore it. First, it affects semantics. E.g., one can insert a string with greater length into a cache/table without any problems. Second, it limits efficiency of our default configuration. E.g., index inline cannot be applied to {{String}} data type as we cannot guess it's length.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)