Re: "not null" constraint support

[Vladimir Ozerov <vo...@gridgain.com>]

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 <pt...@apache.org>
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 <vo...@gridgain.com>
> 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 <
> > sergey.chugunov@gmail.com>
> > 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 <pt...@apache.org>
> > > 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 <
> > > zkillingjar@gmail.com
> > > > >
> > > > 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 <pt...@apache.org>:
> > > > > > 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 <dmagda@apache.org
> >
> > > > 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 <
> > > > > zkillingjar@gmail.com>
> > > > > >> 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
> > > > > >>
> > > > > >>
> > > > >
> > > >
> > >
> >
>