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 <[email protected]> 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 <[email protected]> > 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 <[email protected]> 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 < > [email protected]> > >> 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 <[email protected]> wrote: > >>> > >>>> Create a test case similar to SqlParserTest.testSqlOptions... the rest > >>>> should follow... > >>>> > >>>> On Tue, Jul 28, 2015 at 6:07 AM, Jacques Nadeau <[email protected]> > >>>> wrote: > >>>>> Ps, the word at the beginning of that email should have been "if" > >>>>> On Jul 28, 2015 6:06 AM, "Jacques Nadeau" <[email protected]> > 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" <[email protected]> > >>>> 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 > >>>>>> > >>>>>> > >>>> > >> > >> > >
