> 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 <voze...@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.chugu...@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 <ptupit...@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 <
> > zkilling...@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 <ptupit...@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 <dma...@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 <
> > > > zkilling...@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
> > > > >>
> > > > >>
> > > >
> > >
> >
>
