"not null" constraint support

[Sergey Kalashnikov]

Hi Igniters,

I am working on the ticket
https://issues.apache.org/jira/browse/IGNITE-5648, which purpose is to
add support for NOT NULL constraint for any fields of key or value
stored in Ignite cache.

I would appreciate your comments on the design approach I have described below.

In my opinion, such checks should be enforced both by SQL DML and
cache API to be consistent.

Here is my analysis of what needs to be done.

1. First, the SQL CREATE table will not throw exception anymore
whenever it encounters a column with "not null" modifier.
Instead, the resulting QueryEntity will now indicate which fields have
such modifier.

The proposed way of doing this is the following:
class QueryEntity {
    ...
    Set<String> notNullFields;
}

Since QueryEntity is a part of public api, it becomes possible to
configure this constraint not only via DDL CREATE TABLE.

2. Then we need a special method on GridQueryProcessor that for the
given cacheName, key and value would perform the following things:
- Get a GridQueryTypeDescriptor that corresponds to given value type;
- Delegate that GridQueryTypeDescriptor a task to validate given key and value;
- Type descriptor would itself delegate the validation to its
GridQueryProperties that have "not null" constraint.

3. To enforce the constraints, the validation method should be called
- In GridNearAtomicSingleUpdateFuture.mapSingleUpdate() and
GridNearAtomicUpdateFuture.mapUpdate() when operation is CREATE or
UPDATE.
That would cover putIfAbsent(), getAndPut(), getAndPutIfAbsent(),
replace(), getAndReplace(), putAll() operations on atomic cache.
And in GridNearTxLocal.enlistWriteEntry() when operation is CREATE or
UPDATE for the case of transactional cache.

- Right after EntryProcessor.process() in
GridCacheMapEntry.AtomicCacheUpdateClosure.runEntryProcessor() as part
of invoke(), invokeAll() operations on atomic cache.
And in GridDhtTxPrepareFuture.onEntriesLocked() for the case of
transactional cache.

4. DML processor changes
  The DMLStatementProcessor methods doInsert(), doUpdate(), doMerge()
must also incorporate such checks to avoid attempts
  to insert values that are known to be rejected by cache.

Thoughts?

--
Best regards,
Sergey

[dmagda]

Hi Sergey,

That will be an excellent addition to DDL features set.

The proposed changes look good to from the public API standpoint.

Alexander P., Sergi, Vovan, please share your thoughts.

—
Denis

> On Jul 20, 2017, at 12:27 PM, Sergey Kalashnikov <[hidden email]> wrote:
>
> Hi Igniters,
>
> I am working on the ticket
> https://issues.apache.org/jira/browse/IGNITE-5648, which purpose is to
> add support for NOT NULL constraint for any fields of key or value
> stored in Ignite cache.
>
> I would appreciate your comments on the design approach I have described below.
>
> In my opinion, such checks should be enforced both by SQL DML and
> cache API to be consistent.
>
> Here is my analysis of what needs to be done.
>
> 1. First, the SQL CREATE table will not throw exception anymore
> whenever it encounters a column with "not null" modifier.
> Instead, the resulting QueryEntity will now indicate which fields have
> such modifier.
>
> The proposed way of doing this is the following:
> class QueryEntity {
>    ...
>    Set<String> notNullFields;
> }
>
> Since QueryEntity is a part of public api, it becomes possible to
> configure this constraint not only via DDL CREATE TABLE.
>
> 2. Then we need a special method on GridQueryProcessor that for the
> given cacheName, key and value would perform the following things:
> - Get a GridQueryTypeDescriptor that corresponds to given value type;
> - Delegate that GridQueryTypeDescriptor a task to validate given key and value;
> - Type descriptor would itself delegate the validation to its
> GridQueryProperties that have "not null" constraint.
>
> 3. To enforce the constraints, the validation method should be called
> - In GridNearAtomicSingleUpdateFuture.mapSingleUpdate() and
> GridNearAtomicUpdateFuture.mapUpdate() when operation is CREATE or
> UPDATE.
> That would cover putIfAbsent(), getAndPut(), getAndPutIfAbsent(),
> replace(), getAndReplace(), putAll() operations on atomic cache.
> And in GridNearTxLocal.enlistWriteEntry() when operation is CREATE or
> UPDATE for the case of transactional cache.
>
> - Right after EntryProcessor.process() in
> GridCacheMapEntry.AtomicCacheUpdateClosure.runEntryProcessor() as part
> of invoke(), invokeAll() operations on atomic cache.
> And in GridDhtTxPrepareFuture.onEntriesLocked() for the case of
> transactional cache.
>
> 4. DML processor changes
>  The DMLStatementProcessor methods doInsert(), doUpdate(), doMerge()
> must also incorporate such checks to avoid attempts
>  to insert values that are known to be rejected by cache.
>
> Thoughts?
>
> --
> Best regards,
> Sergey

[Pavel Tupitsyn]

Hi Sergey,

This one looks not very good to me:

> class QueryEntity {
>     ...
>     Set<String> notNullFields;
> }

What if there are more constraints in future? UNIQUE, CHECK, etc etc?

Instead we could do something like

    Set<QueryConstraint> constraints;

Which is easily extendable in future.

Thoughts?

Pavel

On Thu, Jul 20, 2017 at 11:17 PM, Denis Magda <[hidden email]> wrote:

> Hi Sergey,
>
> That will be an excellent addition to DDL features set.
>
> The proposed changes look good to from the public API standpoint.
>
> Alexander P., Sergi, Vovan, please share your thoughts.
>
> —
> Denis
>
> > On Jul 20, 2017, at 12:27 PM, Sergey Kalashnikov <[hidden email]>
> wrote:
> >
> > Hi Igniters,
> >
> > I am working on the ticket
> > https://issues.apache.org/jira/browse/IGNITE-5648, which purpose is to
> > add support for NOT NULL constraint for any fields of key or value
> > stored in Ignite cache.
> >
> > I would appreciate your comments on the design approach I have described
> below.
> >
> > In my opinion, such checks should be enforced both by SQL DML and
> > cache API to be consistent.
> >
> > Here is my analysis of what needs to be done.
> >
> > 1. First, the SQL CREATE table will not throw exception anymore
> > whenever it encounters a column with "not null" modifier.
> > Instead, the resulting QueryEntity will now indicate which fields have
> > such modifier.
> >
> > The proposed way of doing this is the following:
> > class QueryEntity {
> >    ...
> >    Set<String> notNullFields;
> > }
> >
> > Since QueryEntity is a part of public api, it becomes possible to
> > configure this constraint not only via DDL CREATE TABLE.
> >
> > 2. Then we need a special method on GridQueryProcessor that for the
> > given cacheName, key and value would perform the following things:
> > - Get a GridQueryTypeDescriptor that corresponds to given value type;
> > - Delegate that GridQueryTypeDescriptor a task to validate given key and
> value;
> > - Type descriptor would itself delegate the validation to its
> > GridQueryProperties that have "not null" constraint.
> >
> > 3. To enforce the constraints, the validation method should be called
> > - In GridNearAtomicSingleUpdateFuture.mapSingleUpdate() and
> > GridNearAtomicUpdateFuture.mapUpdate() when operation is CREATE or
> > UPDATE.
> > That would cover putIfAbsent(), getAndPut(), getAndPutIfAbsent(),
> > replace(), getAndReplace(), putAll() operations on atomic cache.
> > And in GridNearTxLocal.enlistWriteEntry() when operation is CREATE or
> > UPDATE for the case of transactional cache.
> >
> > - Right after EntryProcessor.process() in
> > GridCacheMapEntry.AtomicCacheUpdateClosure.runEntryProcessor() as part
> > of invoke(), invokeAll() operations on atomic cache.
> > And in GridDhtTxPrepareFuture.onEntriesLocked() for the case of
> > transactional cache.
> >
> > 4. DML processor changes
> >  The DMLStatementProcessor methods doInsert(), doUpdate(), doMerge()
> > must also incorporate such checks to avoid attempts
> >  to insert values that are known to be rejected by cache.
> >
> > Thoughts?
> >
> > --
> > Best regards,
> > Sergey
>
>

[Sergey Kalashnikov]

Hi Pavel,

Good point! What if we make it the following way?

class QueryEntity {
    ...

    /** All constraints to be enforced on this QueryEntity. */
    Set<QueryConstraint> constraint;
}

/** Describes constraints that affect one or more fields. */
class QueryConstraint {
    /** Names of fields to be checked. */
    Set<String> fields;

    /** Indicates whether check for null is required. */
    boolean notNull;

    ... here we would add more flags corresponding to other constraints ...
}

--
Best Regards,
Sergey


2017-07-21 10:54 GMT+03:00 Pavel Tupitsyn <[hidden email]>:

> Hi Sergey,
>
> This one looks not very good to me:
>
>> class QueryEntity {
>>     ...
>>     Set<String> notNullFields;
>> }
>
> What if there are more constraints in future? UNIQUE, CHECK, etc etc?
>
> Instead we could do something like
>
>     Set<QueryConstraint> constraints;
>
> Which is easily extendable in future.
>
> Thoughts?
>
> Pavel
>
> On Thu, Jul 20, 2017 at 11:17 PM, Denis Magda <[hidden email]> wrote:
>
>> Hi Sergey,
>>
>> That will be an excellent addition to DDL features set.
>>
>> The proposed changes look good to from the public API standpoint.
>>
>> Alexander P., Sergi, Vovan, please share your thoughts.
>>
>> —
>> Denis
>>
>> > On Jul 20, 2017, at 12:27 PM, Sergey Kalashnikov <[hidden email]>
>> wrote:
>> >
>> > Hi Igniters,
>> >
>> > I am working on the ticket
>> > https://issues.apache.org/jira/browse/IGNITE-5648, which purpose is to
>> > add support for NOT NULL constraint for any fields of key or value
>> > stored in Ignite cache.
>> >
>> > I would appreciate your comments on the design approach I have described
>> below.
>> >
>> > In my opinion, such checks should be enforced both by SQL DML and
>> > cache API to be consistent.
>> >
>> > Here is my analysis of what needs to be done.
>> >
>> > 1. First, the SQL CREATE table will not throw exception anymore
>> > whenever it encounters a column with "not null" modifier.
>> > Instead, the resulting QueryEntity will now indicate which fields have
>> > such modifier.
>> >
>> > The proposed way of doing this is the following:
>> > class QueryEntity {
>> >    ...
>> >    Set<String> notNullFields;
>> > }
>> >
>> > Since QueryEntity is a part of public api, it becomes possible to
>> > configure this constraint not only via DDL CREATE TABLE.
>> >
>> > 2. Then we need a special method on GridQueryProcessor that for the
>> > given cacheName, key and value would perform the following things:
>> > - Get a GridQueryTypeDescriptor that corresponds to given value type;
>> > - Delegate that GridQueryTypeDescriptor a task to validate given key and
>> value;
>> > - Type descriptor would itself delegate the validation to its
>> > GridQueryProperties that have "not null" constraint.
>> >
>> > 3. To enforce the constraints, the validation method should be called
>> > - In GridNearAtomicSingleUpdateFuture.mapSingleUpdate() and
>> > GridNearAtomicUpdateFuture.mapUpdate() when operation is CREATE or
>> > UPDATE.
>> > That would cover putIfAbsent(), getAndPut(), getAndPutIfAbsent(),
>> > replace(), getAndReplace(), putAll() operations on atomic cache.
>> > And in GridNearTxLocal.enlistWriteEntry() when operation is CREATE or
>> > UPDATE for the case of transactional cache.
>> >
>> > - Right after EntryProcessor.process() in
>> > GridCacheMapEntry.AtomicCacheUpdateClosure.runEntryProcessor() as part
>> > of invoke(), invokeAll() operations on atomic cache.
>> > And in GridDhtTxPrepareFuture.onEntriesLocked() for the case of
>> > transactional cache.
>> >
>> > 4. DML processor changes
>> >  The DMLStatementProcessor methods doInsert(), doUpdate(), doMerge()
>> > must also incorporate such checks to avoid attempts
>> >  to insert values that are known to be rejected by cache.
>> >
>> > Thoughts?
>> >
>> > --
>> > Best regards,
>> > Sergey
>>
>>

[Pavel Tupitsyn]

Sergey, looks good to me!

I thought of something a bit different, where there is a base class
and each constraint type inherits it, but your design is actually better.

Pavel

On Fri, Jul 21, 2017 at 6:38 PM, Sergey Kalashnikov <[hidden email]>
wrote:

> Hi Pavel,
>
> Good point! What if we make it the following way?
>
> class QueryEntity {
>     ...
>
>     /** All constraints to be enforced on this QueryEntity. */
>     Set<QueryConstraint> constraint;
> }
>
> /** Describes constraints that affect one or more fields. */
> class QueryConstraint {
>     /** Names of fields to be checked. */
>     Set<String> fields;
>
>     /** Indicates whether check for null is required. */
>     boolean notNull;
>
>     ... here we would add more flags corresponding to other constraints ...
> }
>
> --
> Best Regards,
> Sergey
>
>
> 2017-07-21 10:54 GMT+03:00 Pavel Tupitsyn <[hidden email]>:
> > Hi Sergey,
> >
> > This one looks not very good to me:
> >
> >> class QueryEntity {
> >>     ...
> >>     Set<String> notNullFields;
> >> }
> >
> > What if there are more constraints in future? UNIQUE, CHECK, etc etc?
> >
> > Instead we could do something like
> >
> >     Set<QueryConstraint> constraints;
> >
> > Which is easily extendable in future.
> >
> > Thoughts?
> >
> > Pavel
> >
> > On Thu, Jul 20, 2017 at 11:17 PM, Denis Magda <[hidden email]> wrote:
> >
> >> Hi Sergey,
> >>
> >> That will be an excellent addition to DDL features set.
> >>
> >> The proposed changes look good to from the public API standpoint.
> >>
> >> Alexander P., Sergi, Vovan, please share your thoughts.
> >>
> >> —
> >> Denis
> >>
> >> > On Jul 20, 2017, at 12:27 PM, Sergey Kalashnikov <
> [hidden email]>
> >> wrote:
> >> >
> >> > Hi Igniters,
> >> >
> >> > I am working on the ticket
> >> > https://issues.apache.org/jira/browse/IGNITE-5648, which purpose is
> to
> >> > add support for NOT NULL constraint for any fields of key or value
> >> > stored in Ignite cache.
> >> >
> >> > I would appreciate your comments on the design approach I have
> described
> >> below.
> >> >
> >> > In my opinion, such checks should be enforced both by SQL DML and
> >> > cache API to be consistent.
> >> >
> >> > Here is my analysis of what needs to be done.
> >> >
> >> > 1. First, the SQL CREATE table will not throw exception anymore
> >> > whenever it encounters a column with "not null" modifier.
> >> > Instead, the resulting QueryEntity will now indicate which fields have
> >> > such modifier.
> >> >
> >> > The proposed way of doing this is the following:
> >> > class QueryEntity {
> >> >    ...
> >> >    Set<String> notNullFields;
> >> > }
> >> >
> >> > Since QueryEntity is a part of public api, it becomes possible to
> >> > configure this constraint not only via DDL CREATE TABLE.
> >> >
> >> > 2. Then we need a special method on GridQueryProcessor that for the
> >> > given cacheName, key and value would perform the following things:
> >> > - Get a GridQueryTypeDescriptor that corresponds to given value type;
> >> > - Delegate that GridQueryTypeDescriptor a task to validate given key
> and
> >> value;
> >> > - Type descriptor would itself delegate the validation to its
> >> > GridQueryProperties that have "not null" constraint.
> >> >
> >> > 3. To enforce the constraints, the validation method should be called
> >> > - In GridNearAtomicSingleUpdateFuture.mapSingleUpdate() and
> >> > GridNearAtomicUpdateFuture.mapUpdate() when operation is CREATE or
> >> > UPDATE.
> >> > That would cover putIfAbsent(), getAndPut(), getAndPutIfAbsent(),
> >> > replace(), getAndReplace(), putAll() operations on atomic cache.
> >> > And in GridNearTxLocal.enlistWriteEntry() when operation is CREATE or
> >> > UPDATE for the case of transactional cache.
> >> >
> >> > - Right after EntryProcessor.process() in
> >> > GridCacheMapEntry.AtomicCacheUpdateClosure.runEntryProcessor() as
> part
> >> > of invoke(), invokeAll() operations on atomic cache.
> >> > And in GridDhtTxPrepareFuture.onEntriesLocked() for the case of
> >> > transactional cache.
> >> >
> >> > 4. DML processor changes
> >> >  The DMLStatementProcessor methods doInsert(), doUpdate(), doMerge()
> >> > must also incorporate such checks to avoid attempts
> >> >  to insert values that are known to be rejected by cache.
> >> >
> >> > Thoughts?
> >> >
> >> > --
> >> > Best regards,
> >> > Sergey
> >>
> >>
>

[Sergey Chugunov]

Sergey,

It may be a good idea to distinguish between field constraints (like "not
null" one) which can be applied to only one field; and more complex
constraints that involves more than one field.

In case of field constraints it is better to simplify our model and allow
only one field to appear inside constraint entity class.

Something like this:
class QueryEntity {
    ...

    /** All constraints to be enforced on this QueryEntity. */
    Set<FieldConstraint> fieldConstraints;
}

/** Describes all constraints that affect the field. */
class FieldConstraint {
    /** Field name to be examined against all its constraints. */
    String fieldName;

    /** Indicates whether check for null is required. */
    boolean notNull;

    ... here we would add more flags corresponding to other constraints ...
}

This model has a flaw that if we want to enforce the same constraint on
many fields we must define FieldConstraint entity for each of these fields.
But it has its advantages too: first of all, its simplicity and therefore
obvious usage patterns; and easy to implement validation.

Thanks,
Sergey Ch.


On Fri, Jul 21, 2017 at 6:43 PM, Pavel Tupitsyn <[hidden email]>
wrote:

> Sergey, looks good to me!
>
> I thought of something a bit different, where there is a base class
> and each constraint type inherits it, but your design is actually better.
>
> Pavel
>
> On Fri, Jul 21, 2017 at 6:38 PM, Sergey Kalashnikov <[hidden email]
> >
> wrote:
>
> > Hi Pavel,
> >
> > Good point! What if we make it the following way?
> >
> > class QueryEntity {
> >     ...
> >
> >     /** All constraints to be enforced on this QueryEntity. */
> >     Set<QueryConstraint> constraint;
> > }
> >
> > /** Describes constraints that affect one or more fields. */
> > class QueryConstraint {
> >     /** Names of fields to be checked. */
> >     Set<String> fields;
> >
> >     /** Indicates whether check for null is required. */
> >     boolean notNull;
> >
> >     ... here we would add more flags corresponding to other constraints
> ...
> > }
> >
> > --
> > Best Regards,
> > Sergey
> >
> >
> > 2017-07-21 10:54 GMT+03:00 Pavel Tupitsyn <[hidden email]>:
> > > Hi Sergey,
> > >
> > > This one looks not very good to me:
> > >
> > >> class QueryEntity {
> > >>     ...
> > >>     Set<String> notNullFields;
> > >> }
> > >
> > > What if there are more constraints in future? UNIQUE, CHECK, etc etc?
> > >
> > > Instead we could do something like
> > >
> > >     Set<QueryConstraint> constraints;
> > >
> > > Which is easily extendable in future.
> > >
> > > Thoughts?
> > >
> > > Pavel
> > >
> > > On Thu, Jul 20, 2017 at 11:17 PM, Denis Magda <[hidden email]>
> wrote:
> > >
> > >> Hi Sergey,
> > >>
> > >> That will be an excellent addition to DDL features set.
> > >>
> > >> The proposed changes look good to from the public API standpoint.
> > >>
> > >> Alexander P., Sergi, Vovan, please share your thoughts.
> > >>
> > >> —
> > >> Denis
> > >>
> > >> > On Jul 20, 2017, at 12:27 PM, Sergey Kalashnikov <
> > [hidden email]>
> > >> wrote:
> > >> >
> > >> > Hi Igniters,
> > >> >
> > >> > I am working on the ticket
> > >> > https://issues.apache.org/jira/browse/IGNITE-5648, which purpose is
> > to
> > >> > add support for NOT NULL constraint for any fields of key or value
> > >> > stored in Ignite cache.
> > >> >
> > >> > I would appreciate your comments on the design approach I have
> > described
> > >> below.
> > >> >
> > >> > In my opinion, such checks should be enforced both by SQL DML and
> > >> > cache API to be consistent.
> > >> >
> > >> > Here is my analysis of what needs to be done.
> > >> >
> > >> > 1. First, the SQL CREATE table will not throw exception anymore
> > >> > whenever it encounters a column with "not null" modifier.
> > >> > Instead, the resulting QueryEntity will now indicate which fields
> have
> > >> > such modifier.
> > >> >
> > >> > The proposed way of doing this is the following:
> > >> > class QueryEntity {
> > >> >    ...
> > >> >    Set<String> notNullFields;
> > >> > }
> > >> >
> > >> > Since QueryEntity is a part of public api, it becomes possible to
> > >> > configure this constraint not only via DDL CREATE TABLE.
> > >> >
> > >> > 2. Then we need a special method on GridQueryProcessor that for the
> > >> > given cacheName, key and value would perform the following things:
> > >> > - Get a GridQueryTypeDescriptor that corresponds to given value
> type;
> > >> > - Delegate that GridQueryTypeDescriptor a task to validate given key
> > and
> > >> value;
> > >> > - Type descriptor would itself delegate the validation to its
> > >> > GridQueryProperties that have "not null" constraint.
> > >> >
> > >> > 3. To enforce the constraints, the validation method should be
> called
> > >> > - In GridNearAtomicSingleUpdateFuture.mapSingleUpdate() and
> > >> > GridNearAtomicUpdateFuture.mapUpdate() when operation is CREATE or
> > >> > UPDATE.
> > >> > That would cover putIfAbsent(), getAndPut(), getAndPutIfAbsent(),
> > >> > replace(), getAndReplace(), putAll() operations on atomic cache.
> > >> > And in GridNearTxLocal.enlistWriteEntry() when operation is CREATE
> or
> > >> > UPDATE for the case of transactional cache.
> > >> >
> > >> > - Right after EntryProcessor.process() in
> > >> > GridCacheMapEntry.AtomicCacheUpdateClosure.runEntryProcessor() as
> > part
> > >> > of invoke(), invokeAll() operations on atomic cache.
> > >> > And in GridDhtTxPrepareFuture.onEntriesLocked() for the case of
> > >> > transactional cache.
> > >> >
> > >> > 4. DML processor changes
> > >> >  The DMLStatementProcessor methods doInsert(), doUpdate(), doMerge()
> > >> > must also incorporate such checks to avoid attempts
> > >> >  to insert values that are known to be rejected by cache.
> > >> >
> > >> > Thoughts?
> > >> >
> > >> > --
> > >> > Best regards,
> > >> > Sergey
> > >>
> > >>
> >
>

[Vladimir Ozerov]

Guys,

QueryEntity is already too complex. We will deprecate it soon in favor of
brand new SQL API. No need to design FieldConstraint or something similar
at the moment, let's just stick to "Set<String> notNullFields".

On Fri, Jul 21, 2017 at 7:08 PM, Sergey Chugunov <[hidden email]>
wrote:

> Sergey,
>
> It may be a good idea to distinguish between field constraints (like "not
> null" one) which can be applied to only one field; and more complex
> constraints that involves more than one field.
>
> In case of field constraints it is better to simplify our model and allow
> only one field to appear inside constraint entity class.
>
> Something like this:
> class QueryEntity {
>     ...
>
>     /** All constraints to be enforced on this QueryEntity. */
>     Set<FieldConstraint> fieldConstraints;
> }
>
> /** Describes all constraints that affect the field. */
> class FieldConstraint {
>     /** Field name to be examined against all its constraints. */
>     String fieldName;
>
>     /** Indicates whether check for null is required. */
>     boolean notNull;
>
>     ... here we would add more flags corresponding to other constraints ...
> }
>
> This model has a flaw that if we want to enforce the same constraint on
> many fields we must define FieldConstraint entity for each of these fields.
> But it has its advantages too: first of all, its simplicity and therefore
> obvious usage patterns; and easy to implement validation.
>
> Thanks,
> Sergey Ch.
>
>
> On Fri, Jul 21, 2017 at 6:43 PM, Pavel Tupitsyn <[hidden email]>
> wrote:
>
> > Sergey, looks good to me!
> >
> > I thought of something a bit different, where there is a base class
> > and each constraint type inherits it, but your design is actually better.
> >
> > Pavel
> >
> > On Fri, Jul 21, 2017 at 6:38 PM, Sergey Kalashnikov <
> [hidden email]
> > >
> > wrote:
> >
> > > Hi Pavel,
> > >
> > > Good point! What if we make it the following way?
> > >
> > > class QueryEntity {
> > >     ...
> > >
> > >     /** All constraints to be enforced on this QueryEntity. */
> > >     Set<QueryConstraint> constraint;
> > > }
> > >
> > > /** Describes constraints that affect one or more fields. */
> > > class QueryConstraint {
> > >     /** Names of fields to be checked. */
> > >     Set<String> fields;
> > >
> > >     /** Indicates whether check for null is required. */
> > >     boolean notNull;
> > >
> > >     ... here we would add more flags corresponding to other constraints
> > ...
> > > }
> > >
> > > --
> > > Best Regards,
> > > Sergey
> > >
> > >
> > > 2017-07-21 10:54 GMT+03:00 Pavel Tupitsyn <[hidden email]>:
> > > > Hi Sergey,
> > > >
> > > > This one looks not very good to me:
> > > >
> > > >> class QueryEntity {
> > > >>     ...
> > > >>     Set<String> notNullFields;
> > > >> }
> > > >
> > > > What if there are more constraints in future? UNIQUE, CHECK, etc etc?
> > > >
> > > > Instead we could do something like
> > > >
> > > >     Set<QueryConstraint> constraints;
> > > >
> > > > Which is easily extendable in future.
> > > >
> > > > Thoughts?
> > > >
> > > > Pavel
> > > >
> > > > On Thu, Jul 20, 2017 at 11:17 PM, Denis Magda <[hidden email]>
> > wrote:
> > > >
> > > >> Hi Sergey,
> > > >>
> > > >> That will be an excellent addition to DDL features set.
> > > >>
> > > >> The proposed changes look good to from the public API standpoint.
> > > >>
> > > >> Alexander P., Sergi, Vovan, please share your thoughts.
> > > >>
> > > >> —
> > > >> Denis
> > > >>
> > > >> > On Jul 20, 2017, at 12:27 PM, Sergey Kalashnikov <
> > > [hidden email]>
> > > >> wrote:
> > > >> >
> > > >> > Hi Igniters,
> > > >> >
> > > >> > I am working on the ticket
> > > >> > https://issues.apache.org/jira/browse/IGNITE-5648, which purpose
> is
> > > to
> > > >> > add support for NOT NULL constraint for any fields of key or value
> > > >> > stored in Ignite cache.
> > > >> >
> > > >> > I would appreciate your comments on the design approach I have
> > > described
> > > >> below.
> > > >> >
> > > >> > In my opinion, such checks should be enforced both by SQL DML and
> > > >> > cache API to be consistent.
> > > >> >
> > > >> > Here is my analysis of what needs to be done.
> > > >> >
> > > >> > 1. First, the SQL CREATE table will not throw exception anymore
> > > >> > whenever it encounters a column with "not null" modifier.
> > > >> > Instead, the resulting QueryEntity will now indicate which fields
> > have
> > > >> > such modifier.
> > > >> >
> > > >> > The proposed way of doing this is the following:
> > > >> > class QueryEntity {
> > > >> >    ...
> > > >> >    Set<String> notNullFields;
> > > >> > }
> > > >> >
> > > >> > Since QueryEntity is a part of public api, it becomes possible to
> > > >> > configure this constraint not only via DDL CREATE TABLE.
> > > >> >
> > > >> > 2. Then we need a special method on GridQueryProcessor that for
> the
> > > >> > given cacheName, key and value would perform the following things:
> > > >> > - Get a GridQueryTypeDescriptor that corresponds to given value
> > type;
> > > >> > - Delegate that GridQueryTypeDescriptor a task to validate given
> key
> > > and
> > > >> value;
> > > >> > - Type descriptor would itself delegate the validation to its
> > > >> > GridQueryProperties that have "not null" constraint.
> > > >> >
> > > >> > 3. To enforce the constraints, the validation method should be
> > called
> > > >> > - In GridNearAtomicSingleUpdateFuture.mapSingleUpdate() and
> > > >> > GridNearAtomicUpdateFuture.mapUpdate() when operation is CREATE
> or
> > > >> > UPDATE.
> > > >> > That would cover putIfAbsent(), getAndPut(), getAndPutIfAbsent(),
> > > >> > replace(), getAndReplace(), putAll() operations on atomic cache.
> > > >> > And in GridNearTxLocal.enlistWriteEntry() when operation is
> CREATE
> > or
> > > >> > UPDATE for the case of transactional cache.
> > > >> >
> > > >> > - Right after EntryProcessor.process() in
> > > >> > GridCacheMapEntry.AtomicCacheUpdateClosure.runEntryProcessor() as
> > > part
> > > >> > of invoke(), invokeAll() operations on atomic cache.
> > > >> > And in GridDhtTxPrepareFuture.onEntriesLocked() for the case of
> > > >> > transactional cache.
> > > >> >
> > > >> > 4. DML processor changes
> > > >> >  The DMLStatementProcessor methods doInsert(), doUpdate(),
> doMerge()
> > > >> > must also incorporate such checks to avoid attempts
> > > >> >  to insert values that are known to be rejected by cache.
> > > >> >
> > > >> > Thoughts?
> > > >> >
> > > >> > --
> > > >> > Best regards,
> > > >> > Sergey
> > > >>
> > > >>
> > >
> >
>

[Pavel Tupitsyn]

> QueryEntity is already too complex
> let's make it even more complex

We already see that String-based approach is bad:
* LinkedHashMap<String, String> fields
* Set<String> keyFields
* Map<String, String> aliases

Which should have been just QueryField { name, isKey, alias }.
Let's learn from our mistakes and do constraints right.


> we will deprecate it soon

This is not a valid argument.
What is "soon"? 3.0 is not soon, and I think we don't deprecate APIs in
minor releases.
Temporary things tend to become permanent very often.

Thanks,
Pavel


On Fri, Jul 21, 2017 at 9:57 PM, Vladimir Ozerov <[hidden email]>
wrote:

> Guys,
>
> QueryEntity is already too complex. We will deprecate it soon in favor of
> brand new SQL API. No need to design FieldConstraint or something similar
> at the moment, let's just stick to "Set<String> notNullFields".
>
> On Fri, Jul 21, 2017 at 7:08 PM, Sergey Chugunov <
> [hidden email]>
> wrote:
>
> > Sergey,
> >
> > It may be a good idea to distinguish between field constraints (like "not
> > null" one) which can be applied to only one field; and more complex
> > constraints that involves more than one field.
> >
> > In case of field constraints it is better to simplify our model and allow
> > only one field to appear inside constraint entity class.
> >
> > Something like this:
> > class QueryEntity {
> >     ...
> >
> >     /** All constraints to be enforced on this QueryEntity. */
> >     Set<FieldConstraint> fieldConstraints;
> > }
> >
> > /** Describes all constraints that affect the field. */
> > class FieldConstraint {
> >     /** Field name to be examined against all its constraints. */
> >     String fieldName;
> >
> >     /** Indicates whether check for null is required. */
> >     boolean notNull;
> >
> >     ... here we would add more flags corresponding to other constraints
> ...
> > }
> >
> > This model has a flaw that if we want to enforce the same constraint on
> > many fields we must define FieldConstraint entity for each of these
> fields.
> > But it has its advantages too: first of all, its simplicity and therefore
> > obvious usage patterns; and easy to implement validation.
> >
> > Thanks,
> > Sergey Ch.
> >
> >
> > On Fri, Jul 21, 2017 at 6:43 PM, Pavel Tupitsyn <[hidden email]>
> > wrote:
> >
> > > Sergey, looks good to me!
> > >
> > > I thought of something a bit different, where there is a base class
> > > and each constraint type inherits it, but your design is actually
> better.
> > >
> > > Pavel
> > >
> > > On Fri, Jul 21, 2017 at 6:38 PM, Sergey Kalashnikov <
> > [hidden email]
> > > >
> > > wrote:
> > >
> > > > Hi Pavel,
> > > >
> > > > Good point! What if we make it the following way?
> > > >
> > > > class QueryEntity {
> > > >     ...
> > > >
> > > >     /** All constraints to be enforced on this QueryEntity. */
> > > >     Set<QueryConstraint> constraint;
> > > > }
> > > >
> > > > /** Describes constraints that affect one or more fields. */
> > > > class QueryConstraint {
> > > >     /** Names of fields to be checked. */
> > > >     Set<String> fields;
> > > >
> > > >     /** Indicates whether check for null is required. */
> > > >     boolean notNull;
> > > >
> > > >     ... here we would add more flags corresponding to other
> constraints
> > > ...
> > > > }
> > > >
> > > > --
> > > > Best Regards,
> > > > Sergey
> > > >
> > > >
> > > > 2017-07-21 10:54 GMT+03:00 Pavel Tupitsyn <[hidden email]>:
> > > > > Hi Sergey,
> > > > >
> > > > > This one looks not very good to me:
> > > > >
> > > > >> class QueryEntity {
> > > > >>     ...
> > > > >>     Set<String> notNullFields;
> > > > >> }
> > > > >
> > > > > What if there are more constraints in future? UNIQUE, CHECK, etc
> etc?
> > > > >
> > > > > Instead we could do something like
> > > > >
> > > > >     Set<QueryConstraint> constraints;
> > > > >
> > > > > Which is easily extendable in future.
> > > > >
> > > > > Thoughts?
> > > > >
> > > > > Pavel
> > > > >
> > > > > On Thu, Jul 20, 2017 at 11:17 PM, Denis Magda <[hidden email]>
> > > wrote:
> > > > >
> > > > >> Hi Sergey,
> > > > >>
> > > > >> That will be an excellent addition to DDL features set.
> > > > >>
> > > > >> The proposed changes look good to from the public API standpoint.
> > > > >>
> > > > >> Alexander P., Sergi, Vovan, please share your thoughts.
> > > > >>
> > > > >> —
> > > > >> Denis
> > > > >>
> > > > >> > On Jul 20, 2017, at 12:27 PM, Sergey Kalashnikov <
> > > > [hidden email]>
> > > > >> wrote:
> > > > >> >
> > > > >> > Hi Igniters,
> > > > >> >
> > > > >> > I am working on the ticket
> > > > >> > https://issues.apache.org/jira/browse/IGNITE-5648, which
> purpose
> > is
> > > > to
> > > > >> > add support for NOT NULL constraint for any fields of key or
> value
> > > > >> > stored in Ignite cache.
> > > > >> >
> > > > >> > I would appreciate your comments on the design approach I have
> > > > described
> > > > >> below.
> > > > >> >
> > > > >> > In my opinion, such checks should be enforced both by SQL DML
> and
> > > > >> > cache API to be consistent.
> > > > >> >
> > > > >> > Here is my analysis of what needs to be done.
> > > > >> >
> > > > >> > 1. First, the SQL CREATE table will not throw exception anymore
> > > > >> > whenever it encounters a column with "not null" modifier.
> > > > >> > Instead, the resulting QueryEntity will now indicate which
> fields
> > > have
> > > > >> > such modifier.
> > > > >> >
> > > > >> > The proposed way of doing this is the following:
> > > > >> > class QueryEntity {
> > > > >> >    ...
> > > > >> >    Set<String> notNullFields;
> > > > >> > }
> > > > >> >
> > > > >> > Since QueryEntity is a part of public api, it becomes possible
> to
> > > > >> > configure this constraint not only via DDL CREATE TABLE.
> > > > >> >
> > > > >> > 2. Then we need a special method on GridQueryProcessor that for
> > the
> > > > >> > given cacheName, key and value would perform the following
> things:
> > > > >> > - Get a GridQueryTypeDescriptor that corresponds to given value
> > > type;
> > > > >> > - Delegate that GridQueryTypeDescriptor a task to validate given
> > key
> > > > and
> > > > >> value;
> > > > >> > - Type descriptor would itself delegate the validation to its
> > > > >> > GridQueryProperties that have "not null" constraint.
> > > > >> >
> > > > >> > 3. To enforce the constraints, the validation method should be
> > > called
> > > > >> > - In GridNearAtomicSingleUpdateFuture.mapSingleUpdate() and
> > > > >> > GridNearAtomicUpdateFuture.mapUpdate() when operation is CREATE
> > or
> > > > >> > UPDATE.
> > > > >> > That would cover putIfAbsent(), getAndPut(),
> getAndPutIfAbsent(),
> > > > >> > replace(), getAndReplace(), putAll() operations on atomic cache.
> > > > >> > And in GridNearTxLocal.enlistWriteEntry() when operation is
> > CREATE
> > > or
> > > > >> > UPDATE for the case of transactional cache.
> > > > >> >
> > > > >> > - Right after EntryProcessor.process() in
> > > > >> > GridCacheMapEntry.AtomicCacheUpdateClosure.runEntryProcessor()
> as
> > > > part
> > > > >> > of invoke(), invokeAll() operations on atomic cache.
> > > > >> > And in GridDhtTxPrepareFuture.onEntriesLocked() for the case of
> > > > >> > transactional cache.
> > > > >> >
> > > > >> > 4. DML processor changes
> > > > >> >  The DMLStatementProcessor methods doInsert(), doUpdate(),
> > doMerge()
> > > > >> > must also incorporate such checks to avoid attempts
> > > > >> >  to insert values that are known to be rejected by cache.
> > > > >> >
> > > > >> > Thoughts?
> > > > >> >
> > > > >> > --
> > > > >> > Best regards,
> > > > >> > Sergey
> > > > >>
> > > > >>
> > > >
> > >
> >
>

[Vladimir Ozerov]

Pavel,

"deprecate" != "delete" or "brake". We are not going to wait for 3.0. New
SQL should be designed carefully taking as mush current and future features
in count as possible. That said, in order to add new concepts like
"constraint" on PAI level we should at least start separate discussion and
consider possible options. Ad-hoc decisions for public API is bad practice.

As far as QueryEntity and not-null constraint, we already understand that
this class is too complex, and what we can do to deliver new feature is to
make it just a bit more complex. This is not very good of course, but it
will let us put tough API design questions aside for a while, especially
provided that this feature will usually be used from "CREATE TABLE" script,
so user will not even have to construct QueryEntity by hand.

On Mon, Jul 24, 2017 at 11:32 AM, Pavel Tupitsyn <[hidden email]>
wrote:

> > QueryEntity is already too complex
> > let's make it even more complex
>
> We already see that String-based approach is bad:
> * LinkedHashMap<String, String> fields
> * Set<String> keyFields
> * Map<String, String> aliases
>
> Which should have been just QueryField { name, isKey, alias }.
> Let's learn from our mistakes and do constraints right.
>
>
> > we will deprecate it soon
>
> This is not a valid argument.
> What is "soon"? 3.0 is not soon, and I think we don't deprecate APIs in
> minor releases.
> Temporary things tend to become permanent very often.
>
> Thanks,
> Pavel
>
>
> On Fri, Jul 21, 2017 at 9:57 PM, Vladimir Ozerov <[hidden email]>
> wrote:
>
> > Guys,
> >
> > QueryEntity is already too complex. We will deprecate it soon in favor of
> > brand new SQL API. No need to design FieldConstraint or something similar
> > at the moment, let's just stick to "Set<String> notNullFields".
> >
> > On Fri, Jul 21, 2017 at 7:08 PM, Sergey Chugunov <
> > [hidden email]>
> > wrote:
> >
> > > Sergey,
> > >
> > > It may be a good idea to distinguish between field constraints (like
> "not
> > > null" one) which can be applied to only one field; and more complex
> > > constraints that involves more than one field.
> > >
> > > In case of field constraints it is better to simplify our model and
> allow
> > > only one field to appear inside constraint entity class.
> > >
> > > Something like this:
> > > class QueryEntity {
> > >     ...
> > >
> > >     /** All constraints to be enforced on this QueryEntity. */
> > >     Set<FieldConstraint> fieldConstraints;
> > > }
> > >
> > > /** Describes all constraints that affect the field. */
> > > class FieldConstraint {
> > >     /** Field name to be examined against all its constraints. */
> > >     String fieldName;
> > >
> > >     /** Indicates whether check for null is required. */
> > >     boolean notNull;
> > >
> > >     ... here we would add more flags corresponding to other constraints
> > ...
> > > }
> > >
> > > This model has a flaw that if we want to enforce the same constraint on
> > > many fields we must define FieldConstraint entity for each of these
> > fields.
> > > But it has its advantages too: first of all, its simplicity and
> therefore
> > > obvious usage patterns; and easy to implement validation.
> > >
> > > Thanks,
> > > Sergey Ch.
> > >
> > >
> > > On Fri, Jul 21, 2017 at 6:43 PM, Pavel Tupitsyn <[hidden email]>
> > > wrote:
> > >
> > > > Sergey, looks good to me!
> > > >
> > > > I thought of something a bit different, where there is a base class
> > > > and each constraint type inherits it, but your design is actually
> > better.
> > > >
> > > > Pavel
> > > >
> > > > On Fri, Jul 21, 2017 at 6:38 PM, Sergey Kalashnikov <
> > > [hidden email]
> > > > >
> > > > wrote:
> > > >
> > > > > Hi Pavel,
> > > > >
> > > > > Good point! What if we make it the following way?
> > > > >
> > > > > class QueryEntity {
> > > > >     ...
> > > > >
> > > > >     /** All constraints to be enforced on this QueryEntity. */
> > > > >     Set<QueryConstraint> constraint;
> > > > > }
> > > > >
> > > > > /** Describes constraints that affect one or more fields. */
> > > > > class QueryConstraint {
> > > > >     /** Names of fields to be checked. */
> > > > >     Set<String> fields;
> > > > >
> > > > >     /** Indicates whether check for null is required. */
> > > > >     boolean notNull;
> > > > >
> > > > >     ... here we would add more flags corresponding to other
> > constraints
> > > > ...
> > > > > }
> > > > >
> > > > > --
> > > > > Best Regards,
> > > > > Sergey
> > > > >
> > > > >
> > > > > 2017-07-21 10:54 GMT+03:00 Pavel Tupitsyn <[hidden email]>:
> > > > > > Hi Sergey,
> > > > > >
> > > > > > This one looks not very good to me:
> > > > > >
> > > > > >> class QueryEntity {
> > > > > >>     ...
> > > > > >>     Set<String> notNullFields;
> > > > > >> }
> > > > > >
> > > > > > What if there are more constraints in future? UNIQUE, CHECK, etc
> > etc?
> > > > > >
> > > > > > Instead we could do something like
> > > > > >
> > > > > >     Set<QueryConstraint> constraints;
> > > > > >
> > > > > > Which is easily extendable in future.
> > > > > >
> > > > > > Thoughts?
> > > > > >
> > > > > > Pavel
> > > > > >
> > > > > > On Thu, Jul 20, 2017 at 11:17 PM, Denis Magda <[hidden email]
> >
> > > > wrote:
> > > > > >
> > > > > >> Hi Sergey,
> > > > > >>
> > > > > >> That will be an excellent addition to DDL features set.
> > > > > >>
> > > > > >> The proposed changes look good to from the public API
> standpoint.
> > > > > >>
> > > > > >> Alexander P., Sergi, Vovan, please share your thoughts.
> > > > > >>
> > > > > >> —
> > > > > >> Denis
> > > > > >>
> > > > > >> > On Jul 20, 2017, at 12:27 PM, Sergey Kalashnikov <
> > > > > [hidden email]>
> > > > > >> wrote:
> > > > > >> >
> > > > > >> > Hi Igniters,
> > > > > >> >
> > > > > >> > I am working on the ticket
> > > > > >> > https://issues.apache.org/jira/browse/IGNITE-5648, which
> > purpose
> > > is
> > > > > to
> > > > > >> > add support for NOT NULL constraint for any fields of key or
> > value
> > > > > >> > stored in Ignite cache.
> > > > > >> >
> > > > > >> > I would appreciate your comments on the design approach I have
> > > > > described
> > > > > >> below.
> > > > > >> >
> > > > > >> > In my opinion, such checks should be enforced both by SQL DML
> > and
> > > > > >> > cache API to be consistent.
> > > > > >> >
> > > > > >> > Here is my analysis of what needs to be done.
> > > > > >> >
> > > > > >> > 1. First, the SQL CREATE table will not throw exception
> anymore
> > > > > >> > whenever it encounters a column with "not null" modifier.
> > > > > >> > Instead, the resulting QueryEntity will now indicate which
> > fields
> > > > have
> > > > > >> > such modifier.
> > > > > >> >
> > > > > >> > The proposed way of doing this is the following:
> > > > > >> > class QueryEntity {
> > > > > >> >    ...
> > > > > >> >    Set<String> notNullFields;
> > > > > >> > }
> > > > > >> >
> > > > > >> > Since QueryEntity is a part of public api, it becomes possible
> > to
> > > > > >> > configure this constraint not only via DDL CREATE TABLE.
> > > > > >> >
> > > > > >> > 2. Then we need a special method on GridQueryProcessor that
> for
> > > the
> > > > > >> > given cacheName, key and value would perform the following
> > things:
> > > > > >> > - Get a GridQueryTypeDescriptor that corresponds to given
> value
> > > > type;
> > > > > >> > - Delegate that GridQueryTypeDescriptor a task to validate
> given
> > > key
> > > > > and
> > > > > >> value;
> > > > > >> > - Type descriptor would itself delegate the validation to its
> > > > > >> > GridQueryProperties that have "not null" constraint.
> > > > > >> >
> > > > > >> > 3. To enforce the constraints, the validation method should be
> > > > called
> > > > > >> > - In GridNearAtomicSingleUpdateFuture.mapSingleUpdate() and
> > > > > >> > GridNearAtomicUpdateFuture.mapUpdate() when operation is
> CREATE
> > > or
> > > > > >> > UPDATE.
> > > > > >> > That would cover putIfAbsent(), getAndPut(),
> > getAndPutIfAbsent(),
> > > > > >> > replace(), getAndReplace(), putAll() operations on atomic
> cache.
> > > > > >> > And in GridNearTxLocal.enlistWriteEntry() when operation is
> > > CREATE
> > > > or
> > > > > >> > UPDATE for the case of transactional cache.
> > > > > >> >
> > > > > >> > - Right after EntryProcessor.process() in
> > > > > >> > GridCacheMapEntry.AtomicCacheUpdateClosure.
> runEntryProcessor()
> > as
> > > > > part
> > > > > >> > of invoke(), invokeAll() operations on atomic cache.
> > > > > >> > And in GridDhtTxPrepareFuture.onEntriesLocked() for the case
> of
> > > > > >> > transactional cache.
> > > > > >> >
> > > > > >> > 4. DML processor changes
> > > > > >> >  The DMLStatementProcessor methods doInsert(), doUpdate(),
> > > doMerge()
> > > > > >> > must also incorporate such checks to avoid attempts
> > > > > >> >  to insert values that are known to be rejected by cache.
> > > > > >> >
> > > > > >> > Thoughts?
> > > > > >> >
> > > > > >> > --
> > > > > >> > Best regards,
> > > > > >> > Sergey
> > > > > >>
> > > > > >>
> > > > >
> > > >
> > >
> >
>