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 > >>>> > >>>> > >> > >
