You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@paimon.apache.org by "askwang (via GitHub)" <gi...@apache.org> on 2024/08/26 11:24:57 UTC
[PR] [spark] Adjust ExpireSnapshotsProcedure param type [paimon]
askwang opened a new pull request, #4059:
URL: https://github.com/apache/paimon/pull/4059
<!-- Please specify the module before the PR name: [core] ... or [flink] ... -->
### Purpose
- change param `older_than` type from TimeStampType to StringType, TimeStampType is not easy to understand and use.
- add new param `time_retained`, which can call `expire_snapshots` periodically. `older_than` is suitable for one-time call.
<!-- What is the purpose of the change -->
### Tests
<!-- List UT and IT cases to verify this change -->
### API and Format
<!-- Does this change affect API or storage format -->
### Documentation
<!-- Does this change introduce a new feature -->
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [spark] Adjust ExpireSnapshotsProcedure param type [paimon]
Posted by "askwang (via GitHub)" <gi...@apache.org>.
askwang commented on code in PR #4059:
URL: https://github.com/apache/paimon/pull/4059#discussion_r1759700951
##########
paimon-spark/paimon-spark-common/src/main/java/org/apache/paimon/spark/procedure/ExpireSnapshotsProcedure.java:
##########
@@ -85,9 +88,21 @@ public InternalRow[] call(InternalRow args) {
if (retainMin != null) {
builder.snapshotRetainMin(retainMin);
}
- if (olderThanMills != null) {
- builder.snapshotTimeRetain(
- Duration.ofMillis(System.currentTimeMillis() - olderThanMills));
+ if (!StringUtils.isNullOrWhitespaceOnly(olderThanStr)) {
+ long olderThanMills;
+ // forward compatibility for timestamp type
Review Comment:
In default, the value of timestamp type is like "1724664000000", it's actually a string that contains numerics. And get it by
`Long olderThanMills = args.isNullAt(3) ? null : args.getLong(3) / 1000;`
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [spark] Adjust ExpireSnapshotsProcedure param type [paimon]
Posted by "askwang (via GitHub)" <gi...@apache.org>.
askwang commented on PR #4059:
URL: https://github.com/apache/paimon/pull/4059#issuecomment-2324594120
help cc @JingsongLi
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [spark] Adjust ExpireSnapshotsProcedure param type [paimon]
Posted by "askwang (via GitHub)" <gi...@apache.org>.
askwang commented on code in PR #4059:
URL: https://github.com/apache/paimon/pull/4059#discussion_r1758468841
##########
paimon-spark/paimon-spark-common/src/main/java/org/apache/paimon/spark/procedure/ExpireSnapshotsProcedure.java:
##########
@@ -42,8 +45,9 @@ public class ExpireSnapshotsProcedure extends BaseProcedure {
ProcedureParameter.required("table", StringType),
ProcedureParameter.optional("retain_max", IntegerType),
ProcedureParameter.optional("retain_min", IntegerType),
- ProcedureParameter.optional("older_than", TimestampType),
- ProcedureParameter.optional("max_deletes", IntegerType)
+ ProcedureParameter.optional("older_than", StringType),
+ ProcedureParameter.optional("max_deletes", IntegerType),
+ ProcedureParameter.optional("time_retained", StringType)
Review Comment:
> older_than and time_retained? Looks not easy to understand, we should just keep one.
These two parameters can not be used together.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [spark] Adjust ExpireSnapshotsProcedure param type [paimon]
Posted by "askwang (via GitHub)" <gi...@apache.org>.
askwang closed pull request #4059: [spark] Adjust ExpireSnapshotsProcedure param type
URL: https://github.com/apache/paimon/pull/4059
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [spark] Adjust ExpireSnapshotsProcedure param type [paimon]
Posted by "askwang (via GitHub)" <gi...@apache.org>.
askwang commented on code in PR #4059:
URL: https://github.com/apache/paimon/pull/4059#discussion_r1766429668
##########
paimon-spark/paimon-spark-common/src/main/java/org/apache/paimon/spark/procedure/ExpireSnapshotsProcedure.java:
##########
@@ -85,9 +88,21 @@ public InternalRow[] call(InternalRow args) {
if (retainMin != null) {
builder.snapshotRetainMin(retainMin);
}
- if (olderThanMills != null) {
- builder.snapshotTimeRetain(
- Duration.ofMillis(System.currentTimeMillis() - olderThanMills));
+ if (!StringUtils.isNullOrWhitespaceOnly(olderThanStr)) {
+ long olderThanMills;
+ // forward compatibility for timestamp type
Review Comment:
got, has only retain StringType
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [spark] Adjust ExpireSnapshotsProcedure param type [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi merged PR #4059:
URL: https://github.com/apache/paimon/pull/4059
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [spark] Adjust ExpireSnapshotsProcedure param type [paimon]
Posted by "askwang (via GitHub)" <gi...@apache.org>.
askwang commented on code in PR #4059:
URL: https://github.com/apache/paimon/pull/4059#discussion_r1758476410
##########
paimon-spark/paimon-spark-common/src/main/java/org/apache/paimon/spark/procedure/ExpireSnapshotsProcedure.java:
##########
@@ -42,8 +45,9 @@ public class ExpireSnapshotsProcedure extends BaseProcedure {
ProcedureParameter.required("table", StringType),
ProcedureParameter.optional("retain_max", IntegerType),
ProcedureParameter.optional("retain_min", IntegerType),
- ProcedureParameter.optional("older_than", TimestampType),
- ProcedureParameter.optional("max_deletes", IntegerType)
+ ProcedureParameter.optional("older_than", StringType),
+ ProcedureParameter.optional("max_deletes", IntegerType),
+ ProcedureParameter.optional("time_retained", StringType)
Review Comment:
> Because there is currently compatibility issue. I recommend using a script to address the issue of periodic calls when invoking from the outer layer.
When using a script to calls periodically, we need to adjust older_than parameter each time. I think it is necessary to keep time_retained, what do you 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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [spark] Adjust ExpireSnapshotsProcedure param type [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #4059:
URL: https://github.com/apache/paimon/pull/4059#discussion_r1758138935
##########
paimon-spark/paimon-spark-common/src/main/java/org/apache/paimon/spark/procedure/ExpireSnapshotsProcedure.java:
##########
@@ -42,8 +45,9 @@ public class ExpireSnapshotsProcedure extends BaseProcedure {
ProcedureParameter.required("table", StringType),
ProcedureParameter.optional("retain_max", IntegerType),
ProcedureParameter.optional("retain_min", IntegerType),
- ProcedureParameter.optional("older_than", TimestampType),
- ProcedureParameter.optional("max_deletes", IntegerType)
+ ProcedureParameter.optional("older_than", StringType),
+ ProcedureParameter.optional("max_deletes", IntegerType),
+ ProcedureParameter.optional("time_retained", StringType)
Review Comment:
older_than and time_retained? Looks not easy to understand, we should just keep one.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [spark] Adjust ExpireSnapshotsProcedure param type [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #4059:
URL: https://github.com/apache/paimon/pull/4059#discussion_r1758141327
##########
paimon-spark/paimon-spark-common/src/main/java/org/apache/paimon/spark/procedure/ExpireSnapshotsProcedure.java:
##########
@@ -42,8 +45,9 @@ public class ExpireSnapshotsProcedure extends BaseProcedure {
ProcedureParameter.required("table", StringType),
ProcedureParameter.optional("retain_max", IntegerType),
ProcedureParameter.optional("retain_min", IntegerType),
- ProcedureParameter.optional("older_than", TimestampType),
- ProcedureParameter.optional("max_deletes", IntegerType)
+ ProcedureParameter.optional("older_than", StringType),
+ ProcedureParameter.optional("max_deletes", IntegerType),
+ ProcedureParameter.optional("time_retained", StringType)
Review Comment:
Because there is currently compatibility issue. I recommend using a script to address the issue of periodic calls when invoking from the outer layer.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [spark] Adjust ExpireSnapshotsProcedure param type [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #4059:
URL: https://github.com/apache/paimon/pull/4059#discussion_r1759693130
##########
paimon-spark/paimon-spark-common/src/main/java/org/apache/paimon/spark/procedure/ExpireSnapshotsProcedure.java:
##########
@@ -85,9 +88,21 @@ public InternalRow[] call(InternalRow args) {
if (retainMin != null) {
builder.snapshotRetainMin(retainMin);
}
- if (olderThanMills != null) {
- builder.snapshotTimeRetain(
- Duration.ofMillis(System.currentTimeMillis() - olderThanMills));
+ if (!StringUtils.isNullOrWhitespaceOnly(olderThanStr)) {
+ long olderThanMills;
+ // forward compatibility for timestamp type
Review Comment:
How to forward compatibility for timestamp type? It is a string instead of a timestamp.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [spark] Adjust ExpireSnapshotsProcedure param type [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #4059:
URL: https://github.com/apache/paimon/pull/4059#discussion_r1762908183
##########
paimon-spark/paimon-spark-common/src/main/java/org/apache/paimon/spark/procedure/ExpireSnapshotsProcedure.java:
##########
@@ -85,9 +88,21 @@ public InternalRow[] call(InternalRow args) {
if (retainMin != null) {
builder.snapshotRetainMin(retainMin);
}
- if (olderThanMills != null) {
- builder.snapshotTimeRetain(
- Duration.ofMillis(System.currentTimeMillis() - olderThanMills));
+ if (!StringUtils.isNullOrWhitespaceOnly(olderThanStr)) {
+ long olderThanMills;
+ // forward compatibility for timestamp type
Review Comment:
`change param older_than type from TimeStampType to StringType, TimeStampType is not easy to understand and use.`
Is this related? Can you show the use cases? How to improve the usage.
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [spark] Adjust ExpireSnapshotsProcedure param type [paimon]
Posted by "askwang (via GitHub)" <gi...@apache.org>.
askwang commented on code in PR #4059:
URL: https://github.com/apache/paimon/pull/4059#discussion_r1758818344
##########
paimon-spark/paimon-spark-common/src/main/java/org/apache/paimon/spark/procedure/ExpireSnapshotsProcedure.java:
##########
@@ -42,8 +45,9 @@ public class ExpireSnapshotsProcedure extends BaseProcedure {
ProcedureParameter.required("table", StringType),
ProcedureParameter.optional("retain_max", IntegerType),
ProcedureParameter.optional("retain_min", IntegerType),
- ProcedureParameter.optional("older_than", TimestampType),
- ProcedureParameter.optional("max_deletes", IntegerType)
+ ProcedureParameter.optional("older_than", StringType),
+ ProcedureParameter.optional("max_deletes", IntegerType),
+ ProcedureParameter.optional("time_retained", StringType)
Review Comment:
Thanks. @JingsongLi Removed time_retained
--
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: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org