Skip to content

HIVE-28763: Iceberg: Support functions while expiring snapshots. - #5643

Merged
ayushtkn merged 2 commits into
apache:masterfrom
ayushtkn:HIVE-28763
Feb 18, 2025
Merged

HIVE-28763: Iceberg: Support functions while expiring snapshots.#5643
ayushtkn merged 2 commits into
apache:masterfrom
ayushtkn:HIVE-28763

Conversation

@ayushtkn

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

Allow specifying functions while expiring snapshots

Why are the changes needed?

Better Usability

Does this PR introduce any user-facing change?

Yes, Expire Snapshots allows valid expressions

Is the change a dependency upgrade?

No

How was this patch tested?

UT

| KW_EXECUTE KW_CHERRY_PICK snapshotId=Number
-> ^(TOK_ALTERTABLE_EXECUTE KW_CHERRY_PICK $snapshotId)
| KW_EXECUTE KW_EXPIRE_SNAPSHOTS KW_BETWEEN (fromTimestamp=StringLiteral) KW_AND (toTimestamp=StringLiteral)
| KW_EXECUTE KW_EXPIRE_SNAPSHOTS KW_BETWEEN

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| KW_EXECUTE KW_EXPIRE_SNAPSHOTS KW_BETWEEN
| KW_EXECUTE KW_EXPIRE_SNAPSHOTS KW_BETWEEN

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

QueryState queryState = new QueryState.Builder().withGenerateNewQueryId(false).withHiveConf(conf).build();
SemanticAnalyzer sem = (SemanticAnalyzer) SemanticAnalyzerFactory.get(queryState, node);
ExprNodeDesc desc = sem.genExprNodeDesc(node, new RowResolver(), false, true);
ExprNodeConstantDesc constantDesc = (ExprNodeConstantDesc) desc;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd check if it is a constant or not. This throws ClassCastException.

shell.executeStatement("ALTER TABLE " + identifier.name() + " EXECUTE EXPIRE_SNAPSHOTS(RAND())");

Additionally, we may check whether the type of constantDesc is acceptable here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a check that desc is of type ExprNodeConstantDesc, whether constantDesc is acceptable or not I think the original logic should take care, It should throw some DateTimeParsing exception, same as in case some one provides non timestamp string

@okumin okumin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sonarqubecloud

Copy link
Copy Markdown

@ayushtkn
ayushtkn merged commit f56bf32 into apache:master Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants