[ 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)