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 <[email protected]> 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 <[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 > > >>>>>> > > >>>>>> > > >>>> > > >> > > >> > > > > >
