You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2020/11/24 15:08:49 UTC
[GitHub] [iceberg] liukun4515 opened a new pull request #1819: SQL extension: expire snapshot
liukun4515 opened a new pull request #1819:
URL: https://github.com/apache/iceberg/pull/1819
support the expire snapshot and some mysql test
syntax:
```
CALL catalog.schema.expire_snapshots(
namespace => 'namespace_name'
table => 'table_name',
older_than => timestamp,
retain_last => number
)
The namespace/table/older_than is required.
The retain_last is optional.
```
issue: [#1597](https://github.com/apache/iceberg/issues/1597)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] liukun4515 closed pull request #1819: [WIP]SQL extension: expire snapshot
Posted by GitBox <gi...@apache.org>.
liukun4515 closed pull request #1819:
URL: https://github.com/apache/iceberg/pull/1819
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on pull request #1819: [WIP]SQL extension: expire snapshot
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #1819:
URL: https://github.com/apache/iceberg/pull/1819#issuecomment-733892986
If we are going to add defaults, I'd say let's go ahead and define table properties now.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #1819: [WIP]SQL extension: expire snapshot
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1819:
URL: https://github.com/apache/iceberg/pull/1819#issuecomment-733859164
If we want to add table config in a follow-up, then I think we should go ahead and use a reasonable default here. If `older_than` is not set, then let's just default it to `System.currentTimeMillis() - (3 days in millis)`. I think that's a good default and we can fix the default interval with a table property after this is done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #1819: [WIP]SQL extension: expire snapshot
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1819:
URL: https://github.com/apache/iceberg/pull/1819#issuecomment-733415844
> If don't give the older_than value, all the snapshots will be dropped.
The current snapshot is never dropped, but you're right that it is easy to drop lots of data doing this.
We've thought about introducing table properties to default these. Seems like a good time to add them. I'm thinking some time interval for keeping snapshots (so expireOlderThan would default to `System.currentTimeMillis() - interval`) and some default number of snapshots to keep.
@aokolnychyi, do you agree?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on pull request #1819: [WIP]SQL extension: expire snapshot
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #1819:
URL: https://github.com/apache/iceberg/pull/1819#issuecomment-733131946
@liukun4515, could you rebase this one? There were a few changes since the draft PR.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on pull request #1819: [WIP]SQL extension: expire snapshot
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #1819:
URL: https://github.com/apache/iceberg/pull/1819#issuecomment-738018968
@rdblue @liukun4515, I've submitted PR #1868 for table properties. Please, take a look.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #1819: [WIP]SQL extension: expire snapshot
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1819:
URL: https://github.com/apache/iceberg/pull/1819#issuecomment-733974712
@aokolnychyi, what about `history.expire.snapshot-age-ms` and `history.expire.min-snapshots`? Those would control the age after which a snapshot can expire (the interval for `expireOlderThan(now - interval)`) and the retain-last as minimum number of snapshots to keep.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #1819: [WIP]SQL extension: expire snapshot
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1819:
URL: https://github.com/apache/iceberg/pull/1819#issuecomment-733892432
> Something that this brings up for me, is how does a user discover the information about these defaults?
I think that we should probably add `docs()` to the stored procedure interface and add a command to show them, like `HELP system.expire_snapshots`. Probably not `DESCRIBE` since that is used for tables and views.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] liukun4515 commented on pull request #1819: [WIP]SQL extension: expire snapshot
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on pull request #1819:
URL: https://github.com/apache/iceberg/pull/1819#issuecomment-739118105
> I took commits done by @liukun4515 from this PR and submitted #1874 with up-to-date code.
thanks, i will close the pull request.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on pull request #1819: [WIP]SQL extension: expire snapshot
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #1819:
URL: https://github.com/apache/iceberg/pull/1819#issuecomment-733432153
I think we can add table props in a follow-up PR.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] liukun4515 commented on a change in pull request #1819: [WIP]SQL extension: expire snapshot
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on a change in pull request #1819:
URL: https://github.com/apache/iceberg/pull/1819#discussion_r530049246
##########
File path: spark3-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestExpireSnapshotProcedure.java
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.spark.extensions;
+
+import com.google.common.collect.Lists;
+import java.sql.Timestamp;
+import java.time.LocalDateTime;
+import java.util.List;
+import java.util.Map;
+import org.apache.iceberg.Snapshot;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class TestExpireSnapshotProcedure extends SparkExtensionsTestBase {
+ public TestExpireSnapshotProcedure(
+ String catalogName,
+ String implementation,
+ Map<String, String> config) {
+ super(catalogName, implementation, config);
+ }
+
+ @After
+ public void removeTables() {
+ sql("DROP TABLE IF EXISTS %s", tableName);
+ }
+
+ @Test
+ public void testExpireSnapshotUsingPostionArgs() {
+ sql("CREATE TABLE %s (id bigint NOT NULL, data string) USING iceberg", tableName);
+ sql("INSERT INTO TABLE %s VALUES (1, 'a')", tableName);
+ Table table = validationCatalog.loadTable(tableIdent);
+ long firstSnapshotTimestamp = System.currentTimeMillis();
+ sql("INSERT INTO TABLE %s VALUES (1, 'a')", tableName);
+ table.refresh();
+ Assert.assertEquals(2, Lists.newArrayList(table.snapshots()).size());
+ Snapshot secondSnapshot = table.currentSnapshot();
+
+ // without the retained last num arg
+ List<Object[]> output = sql(
+ "CALL %s.system.expire_snapshot('%s', '%s', TIMESTAMP '%s')",
+ catalogName, tableIdent.namespace(), tableIdent.name(), new Timestamp(firstSnapshotTimestamp).toString());
+ table.refresh();
+ assertEquals(
+ "Procedure output must match",
+ ImmutableList.of(row(null, firstSnapshotTimestamp)),
+ output);
+ Assert.assertEquals(1, Lists.newArrayList(table.snapshots()).size());
+ Assert.assertEquals(secondSnapshot.snapshotId(), Lists.newArrayList(table.snapshots()).get(0).snapshotId());
+
+ //with the retained last num arg
+ sql("INSERT INTO TABLE %s VALUES (1, 'a')", tableName);
+ table.refresh();
+ Snapshot tailSnapshot = table.currentSnapshot();
+ sql("INSERT INTO TABLE %s VALUES (1, 'a')", tableName);
+ sql("INSERT INTO TABLE %s VALUES (1, 'a')", tableName);
+ table.refresh();
+ Assert.assertEquals(4, Lists.newArrayList(table.snapshots()).size());
+ Snapshot headSnapshot = table.currentSnapshot();
+ long currentTimeStamp = System.currentTimeMillis();
+ int retainedLastNum = 3;
+ output = sql(
+ "CALL %s.system.expire_snapshot('%s', '%s', TIMESTAMP '%s', %d)",
+ catalogName,
+ tableIdent.namespace(),
+ tableIdent.name(),
+ new Timestamp(currentTimeStamp).toString(),
+ retainedLastNum);
+ table.refresh();
+ assertEquals(
+ "Procedure output must match",
+ ImmutableList.of(row(retainedLastNum, currentTimeStamp)),
+ output);
+ Assert.assertEquals(3, Lists.newArrayList(table.snapshots()).size());
+ Assert.assertEquals(tailSnapshot.snapshotId(), Lists.newArrayList(table.snapshots()).get(0).snapshotId());
+ Assert.assertEquals(headSnapshot.snapshotId(), Lists.newArrayList(table.snapshots()).get(2).snapshotId());
+ }
+
+ @Test
+ public void testExpireSnapshotUsingNameArgs() {
+
Review comment:
yes, i will add the test case
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on pull request #1819: [WIP]SQL extension: expire snapshot
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #1819:
URL: https://github.com/apache/iceberg/pull/1819#issuecomment-733432037
The design doc assumed that while both `retain_last` and `older_than` are optional, at least one of them is required to prevent the situation described above. That being said, we can probably support `NOW()` literals later to just keep a given number of snapshots. So maybe we don't have to make `older_than` optional.
@rdblue, I am +1 for adding table props to control the default behavior. I was saving this until we have an idea how a generic VACUUM command is going to look like but we can probably introduce properties for retention right now.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on pull request #1819: [WIP]SQL extension: expire snapshot
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #1819:
URL: https://github.com/apache/iceberg/pull/1819#issuecomment-733155551
Also, per design doc, `older_than` is optional and should be defaulted to `System.currentTimeMillis()` if not set.
Then we can keep only a given number of snapshots:
```
CALL catalog.schema.expire_snapshots(
namespace => 'namespace_name',
table => 'table_name',
retain_last => 100
)
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on pull request #1819: [WIP]SQL extension: expire snapshot
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #1819:
URL: https://github.com/apache/iceberg/pull/1819#issuecomment-738743306
I took commits done by @liukun4515 from this PR and submitted #1874 with up-to-date code.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1819: [WIP]SQL extension: expire snapshot
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1819:
URL: https://github.com/apache/iceberg/pull/1819#discussion_r529801492
##########
File path: spark3-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestExpireSnapshotProcedure.java
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.
+ */
+
+package org.apache.iceberg.spark.extensions;
+
+import com.google.common.collect.Lists;
+import java.sql.Timestamp;
+import java.time.LocalDateTime;
+import java.util.List;
+import java.util.Map;
+import org.apache.iceberg.Snapshot;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class TestExpireSnapshotProcedure extends SparkExtensionsTestBase {
+ public TestExpireSnapshotProcedure(
+ String catalogName,
+ String implementation,
+ Map<String, String> config) {
+ super(catalogName, implementation, config);
+ }
+
+ @After
+ public void removeTables() {
+ sql("DROP TABLE IF EXISTS %s", tableName);
+ }
+
+ @Test
+ public void testExpireSnapshotUsingPostionArgs() {
+ sql("CREATE TABLE %s (id bigint NOT NULL, data string) USING iceberg", tableName);
+ sql("INSERT INTO TABLE %s VALUES (1, 'a')", tableName);
+ Table table = validationCatalog.loadTable(tableIdent);
+ long firstSnapshotTimestamp = System.currentTimeMillis();
+ sql("INSERT INTO TABLE %s VALUES (1, 'a')", tableName);
+ table.refresh();
+ Assert.assertEquals(2, Lists.newArrayList(table.snapshots()).size());
+ Snapshot secondSnapshot = table.currentSnapshot();
+
+ // without the retained last num arg
+ List<Object[]> output = sql(
+ "CALL %s.system.expire_snapshot('%s', '%s', TIMESTAMP '%s')",
+ catalogName, tableIdent.namespace(), tableIdent.name(), new Timestamp(firstSnapshotTimestamp).toString());
+ table.refresh();
+ assertEquals(
+ "Procedure output must match",
+ ImmutableList.of(row(null, firstSnapshotTimestamp)),
+ output);
+ Assert.assertEquals(1, Lists.newArrayList(table.snapshots()).size());
+ Assert.assertEquals(secondSnapshot.snapshotId(), Lists.newArrayList(table.snapshots()).get(0).snapshotId());
+
+ //with the retained last num arg
+ sql("INSERT INTO TABLE %s VALUES (1, 'a')", tableName);
+ table.refresh();
+ Snapshot tailSnapshot = table.currentSnapshot();
+ sql("INSERT INTO TABLE %s VALUES (1, 'a')", tableName);
+ sql("INSERT INTO TABLE %s VALUES (1, 'a')", tableName);
+ table.refresh();
+ Assert.assertEquals(4, Lists.newArrayList(table.snapshots()).size());
+ Snapshot headSnapshot = table.currentSnapshot();
+ long currentTimeStamp = System.currentTimeMillis();
+ int retainedLastNum = 3;
+ output = sql(
+ "CALL %s.system.expire_snapshot('%s', '%s', TIMESTAMP '%s', %d)",
+ catalogName,
+ tableIdent.namespace(),
+ tableIdent.name(),
+ new Timestamp(currentTimeStamp).toString(),
+ retainedLastNum);
+ table.refresh();
+ assertEquals(
+ "Procedure output must match",
+ ImmutableList.of(row(retainedLastNum, currentTimeStamp)),
+ output);
+ Assert.assertEquals(3, Lists.newArrayList(table.snapshots()).size());
+ Assert.assertEquals(tailSnapshot.snapshotId(), Lists.newArrayList(table.snapshots()).get(0).snapshotId());
+ Assert.assertEquals(headSnapshot.snapshotId(), Lists.newArrayList(table.snapshots()).get(2).snapshotId());
+ }
+
+ @Test
+ public void testExpireSnapshotUsingNameArgs() {
+
Review comment:
Will need these tests, I 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] liukun4515 commented on pull request #1819: [WIP]SQL extension: expire snapshot
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on pull request #1819:
URL: https://github.com/apache/iceberg/pull/1819#issuecomment-733405329
> Also, per design doc, `older_than` is optional and should be defaulted to `System.currentTimeMillis()` if not set.
>
> Then we can keep only a given number of snapshots:
>
> ```
> CALL catalog.schema.expire_snapshots(
> namespace => 'namespace_name',
> table => 'table_name',
> retain_last => 100
> )
> ```
The the default `older_than`(`System.currentTime`) is a very dangerous.
If some one not give the `older_than` value, all the snapshots will be dropped.
@chenjunjiedada
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] liukun4515 edited a comment on pull request #1819: [WIP]SQL extension: expire snapshot
Posted by GitBox <gi...@apache.org>.
liukun4515 edited a comment on pull request #1819:
URL: https://github.com/apache/iceberg/pull/1819#issuecomment-733405329
> Also, per design doc, `older_than` is optional and should be defaulted to `System.currentTimeMillis()` if not set.
>
> Then we can keep only a given number of snapshots:
>
> ```
> CALL catalog.schema.expire_snapshots(
> namespace => 'namespace_name',
> table => 'table_name',
> retain_last => 100
> )
> ```
The the default `older_than`(`System.currentTime`) is a very dangerous.
If don't give the `older_than` value, all the snapshots will be dropped.
@chenjunjiedada
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on pull request #1819: [WIP]SQL extension: expire snapshot
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on pull request #1819:
URL: https://github.com/apache/iceberg/pull/1819#issuecomment-733874190
Something that this brings up for me, is how does a user discover the
information about these defaults? Particularly from a full SQL context, I
wonder if it makes sense to create a "Help" command which prints out this
info or returns it as a dataframe. Something like
```help expireSnapshots```
On Wed, Nov 25, 2020 at 11:53 AM Ryan Blue <no...@github.com> wrote:
> If we want to add table config in a follow-up, then I think we should go
> ahead and use a reasonable default here. If older_than is not set, then
> let's just default it to System.currentTimeMillis() - (3 days in millis).
> I think that's a good default and we can fix the default interval with a
> table property after this is done.
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/iceberg/pull/1819#issuecomment-733859164>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AADE2YPIB7YP2VRRYOAEP2LSRVABRANCNFSM4UBAJ5BQ>
> .
>
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] liukun4515 commented on a change in pull request #1819: [WIP]SQL extension: expire snapshot
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on a change in pull request #1819:
URL: https://github.com/apache/iceberg/pull/1819#discussion_r530048949
##########
File path: spark3-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveProcedures.scala
##########
@@ -50,7 +50,8 @@ case class ResolveProcedures(spark: SparkSession) extends Rule[LogicalPlan] with
// optional params should be at the end
procedure.parameters.sliding(2).foreach {
- case Array(previousParam, currentParam) if previousParam.required && !currentParam.required =>
+ // Optional arg must be behind the required arg.
+ case Array(previousParam, currentParam) if !previousParam.required && currentParam.required =>
Review comment:
i will rebase the master code.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1819: [WIP]SQL extension: expire snapshot
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1819:
URL: https://github.com/apache/iceberg/pull/1819#discussion_r529801282
##########
File path: spark3-extensions/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveProcedures.scala
##########
@@ -50,7 +50,8 @@ case class ResolveProcedures(spark: SparkSession) extends Rule[LogicalPlan] with
// optional params should be at the end
procedure.parameters.sliding(2).foreach {
- case Array(previousParam, currentParam) if previousParam.required && !currentParam.required =>
+ // Optional arg must be behind the required arg.
+ case Array(previousParam, currentParam) if !previousParam.required && currentParam.required =>
Review comment:
I think this has already been fixed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org