gianm commented on a change in pull request #12163: URL: https://github.com/apache/druid/pull/12163#discussion_r794050676
########## File path: sql/src/main/codegen/includes/insert.ftl ########## @@ -0,0 +1,65 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +SqlNode DruidSqlInsert() : +{ + SqlNode insertNode; + SqlNode partitionBy = null; + SqlNodeList clusterBy = null; +} +{ + insertNode = SqlInsert() + [ + <PARTITION> <BY> + partitionBy = StringLiteral() Review comment: As to what goes after PARTITION BY, I'm torn as to what's best. Context: today, partitioning must either be a floor of the primary timestamp `__time`, or "all" which means everything goes in a single partition. So I think there are three options. 1. `PARTITION BY 'day'` or `PARTITION BY 'all'`. This is what's in the PR now. 2. `PARTITION BY DAY` or `PARTITION BY ALL`. I suggested this upthread. DAY and ALL are keywords, not expressions. 3. `PARTITION BY FLOOR(__TIME TO DAY)`. I suggested this in #11929. In this case the partition-by is an expression. (I can't make up my mind, I guess, so I suggested two different things at two different times.) I didn't have a suggestion for how to say "partition by all", but IMO it should be explicit and required, so people don't accidentally partition by all when they didn't mean to. (3) is closest to what other databases do, but those other databases generally allow partitioning on things other than time-floor. For us, it'd have to be an error to specify anything other than `FLOOR(__time TO XXX)`. That's what makes me think (2) is good: why make people specify the whole expression if it can't be changed? The obvious answer is that making things explicit is good and also that lining things up with other databases is good. But, still… After all this I'm leaning towards my original suggestion, #3. But anyway. I'm curious what other people think. ########## File path: sql/src/main/codegen/includes/insert.ftl ########## @@ -0,0 +1,65 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +SqlNode DruidSqlInsert() : +{ + SqlNode insertNode; + SqlNode partitionBy = null; + SqlNodeList clusterBy = null; +} +{ + insertNode = SqlInsert() + [ + <PARTITION> <BY> + partitionBy = StringLiteral() Review comment: As to what goes after PARTITION BY, I'm torn as to what's best. Context: today, partitioning must either be a floor of the primary timestamp `__time`, or "all" which means everything goes in a single partition. So I think there are three options. 1. `PARTITION BY 'day'` or `PARTITION BY 'all'`. This is what's in the PR now. 2. `PARTITION BY DAY` or `PARTITION BY ALL`. I suggested this upthread. DAY and ALL are keywords, not expressions. 3. `PARTITION BY FLOOR(__TIME TO DAY)`. I suggested this in #11929. In this case the partition-by is an expression. (I can't make up my mind, I guess, so I suggested two different things at two different times.) I didn't have a suggestion for how to say "partition by all", but IMO it should be explicit and required, so people don't accidentally partition by all when they didn't mean to. (3) is closest to what other databases do, but those other databases generally allow partitioning on things other than time-floor. For us, it'd have to be an error to specify anything other than `FLOOR(__time TO XXX)`. That's what makes me think (2) is good: why make people specify the whole expression if it can't be changed? The obvious answer is that making things explicit is good and also that lining things up with other databases is good. But, still… After all this I'm leaning towards my original suggestion, (3). But anyway. I'm curious what other people think. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
