calcite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jacques Nadeau <jacq...@apache.org>
Subject Re: Support to reset an option
Date Wed, 29 Jul 2015 23:02:25 GMT
If there no other users, I say we just do a breaking change.

Let's worry about compatibility/etc only when we think it is important.

On Wed, Jul 29, 2015 at 3:17 PM, Jason Altekruse <altekrusejason@gmail.com>
wrote:

> I do understand that the most correct version of this fix is just to return
> a different identifier type in these two cases, the same as it is with
> column names. I was just suggesting possibly standardizing on the parse
> tree from these two cases actually returning identical results, as Drill is
> going to need to make these two inter-operate for backward compatibility
> anyway.
>
> On the other hand I don't generally see it as advisable to lose
> information, so it is probably best for these to be handled separately as
> far as calcite is concerned and any cross-compatibility to be managed by
> Drill and other consumers.
>
> On Wed, Jul 29, 2015 at 2:47 PM, Julian Hyde <jhyde@apache.org> wrote:
>
> > You get a different parse tree depending on whether the dots are inside
> or
> > outside the back-ticks.
> >
> >   ALTER SYSTEM SET `foo`.`bar`.`baz` = 1
> >
> > gives a 3-part identifier with parts “foo”, “bar”, “baz", while
> >
> >   ALTER SYSTEM SET `foo.bar.baz` = 1
> >
> > gives a 1-part identifier with name “foo.bar.baz”. I don’t know whether
> > you want the first variant, but it is not supported right now.
> >
> > Julian
> >
> >
> > > On Jul 28, 2015, at 1:40 PM, Jason Altekruse <altekrusejason@gmail.com
> >
> > wrote:
> > >
> > > To support the second case I was mentioning it would actually require a
> > > change in calcite that I was not thinking about, in terms of adding a
> new
> > > compound identifier to the sql parse tree node you show here. I don't
> > think
> > > there is a need for this functionality, I guess the take away is I
> think
> > > that we just need to make sure we use standardized format of the
> > flattened
> > > compound names into this name field when creating the sql parse tree. I
> > > know the parser already hides details of removing backtick, so in all
> > > likelihood it will just work as expected. It would probably just be
> worth
> > > writing unit tests that use both strategies and make sure the names
> > > provided from the parse step are the same.
> > >
> > > On Tue, Jul 28, 2015 at 12:28 PM, Julian Hyde <jhyde@apache.org>
> wrote:
> > >
> > >> Jason,
> > >>
> > >> I think Drill is the only database using this feature so I don’t think
> > >> compatibility is too important.
> > >>
> > >> What change would be needed? We have
> > >>
> > >> public class SqlSetOption extends SqlCall {
> > >>  /** Scope of the assignment. Values "SYSTEM" and "SESSION" are
> typical.
> > >> */
> > >>  String scope;
> > >>
> > >>  String name;
> > >>
> > >>  /** Value of the option. May be a {@link
> > >> org.apache.calcite.sql.SqlLiteral} or
> > >>   * a {@link org.apache.calcite.sql.SqlIdentifier} with one
> > >>   * part. Reserved words (currently just 'ON') are converted to
> > >>   * identifiers by the parser. */
> > >>  SqlNode value;
> > >> }
> > >>
> > >> and we could add a “SqlIdentifier identifier” field and deprecate
> > “name”.
> > >> “name” could continue to hold the last part of the identifier in the
> > short
> > >> term.
> > >>
> > >> If a particular database built using Calcite only allows single-part
> > >> identifiers for options then it can easily throw a validation error.
> > >>
> > >> Julian
> > >>
> > >>
> > >>
> > >>> On Jul 28, 2015, at 12:11 PM, Jason Altekruse <
> > altekrusejason@gmail.com>
> > >> wrote:
> > >>>
> > >>> One note on Jacques point about the compound identifiers. To maintain
> > >>> backwards compatibility we are going to need to fudge these new
> > multipart
> > >>> identifiers together with the old single part identifiers that were
> > using
> > >>> previously.
> > >>>
> > >>> In particular for Drill, this could be as simple as maintaining the
> > >> current
> > >>> data structure, which is just a map between strings for option names
> > and
> > >>> option values themselves, with the extra consideration that compound
> > >> names
> > >>> will need to be canonicalized into a standard singular string. The
> > >>> alternative would be to replace the structure with a map from a
> nested
> > >> name
> > >>> representation to option values, with all of the flattened option
> names
> > >>> registered at the root. This is something that can be handled by each
> > >> user
> > >>> of calcite individually, but I thought it might be worth thinking
> about
> > >> the
> > >>> best way to advise these two cases working together now that there
> is a
> > >>> need for backwards compatibility.
> > >>>
> > >>> On Tue, Jul 28, 2015 at 8:15 AM, Julian Hyde <jhyde@apache.org>
> wrote:
> > >>>
> > >>>> Create a test case similar to SqlParserTest.testSqlOptions... the
> rest
> > >>>> should follow...
> > >>>>
> > >>>> On Tue, Jul 28, 2015 at 6:07 AM, Jacques Nadeau <jacques@apache.org
> >
> > >>>> wrote:
> > >>>>> Ps, the word at the beginning of that email should have been
"if"
> > >>>>> On Jul 28, 2015 6:06 AM, "Jacques Nadeau" <jacques@apache.org>
> > wrote:
> > >>>>>
> > >>>>>> Of you're making changes there anyway,  can you make two
> additional
> > >>>>>> changes:
> > >>>>>>
> > >>>>>> Allow "alter session" to be optional for setting.
> > >>>>>> Allow a multipart identifier (so we don't have to quote
a.b.c
> (same
> > as
> > >>>>>> Schema or column identifiers in project lists).
> > >>>>>>
> > >>>>>> This would substantially improve usability.
> > >>>>>> On Jul 27, 2015 6:53 PM, "Sudheesh Katkam" <skatkam@maprtech.com>
> > >>>> wrote:
> > >>>>>>
> > >>>>>>> Hello developers,
> > >>>>>>>
> > >>>>>>> To support these statements as part of Apache Drill's
SQL parser
> > >>>>>>> extension:
> > >>>>>>>
> > >>>>>>> ALTER SESSION RESET `option name`
> > >>>>>>> ALTER SYSTEM RESET `option name`
> > >>>>>>> ALTER SESSION RESET ALL
> > >>>>>>> ALTER SYSTEM RESET ALL
> > >>>>>>>
> > >>>>>>> I added the required implementation files, including
changes to
> the
> > >>>>>>> config file (keywords, statementParserMethods fields).
However, I
> > >> get a
> > >>>>>>> ParseException because the parser resolves to SetSqlOption
> > statement,
> > >>>> which
> > >>>>>>> I assume is due to a lookahead of 2. How do I go about
resolving
> > >> this?
> > >>>>>>>
> > >>>>>>> Thank you,
> > >>>>>>> Sudheesh
> > >>>>>>
> > >>>>>>
> > >>>>
> > >>
> > >>
> >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message