You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@fluss.apache.org by "MaggieSullivan (via GitHub)" <gi...@apache.org> on 2025/07/03 22:57:30 UTC
[PR] Issue 489 parition key more types apache [fluss]
MaggieSullivan opened a new pull request, #1264:
URL: https://github.com/apache/fluss/pull/1264
<!--
*Thank you very much for contributing to Fluss - we are happy that you want to help us improve Fluss. To help the community review your contribution in the best possible way, please go through the checklist below, which will get the contribution into a shape in which it can be best reviewed.*
## Contribution Checklist
- Make sure that the pull request corresponds to a [GitHub issue](https://github.com/alibaba/fluss/issues). Exceptions are made for typos in JavaDoc or documentation files, which need no issue.
- Name the pull request in the format "[component] Title of the pull request", where *[component]* should be replaced by the name of the component being changed. Typically, this corresponds to the component label assigned to the issue (e.g., [kv], [log], [client], [flink]). Skip *[component]* if you are unsure about which is the best component.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Make sure that the change passes the automated tests, i.e., `mvn clean verify` passes.
- Each pull request should address only one issue, not mix up code from multiple issues.
**(The sections below can be removed for hotfixes or typos)**
-->
### Purpose
This implements the ability for partition keys to use any of the supported types in fluss, not just String.
<!-- Linking this pull request to the issue -->
Linked issue: close #489
Deprecates: https://github.com/apache/fluss/pull/979 (this is updated for apache switchover.)
### Brief change log
Add partition value -> string conversion to support more data types for partition key.
### Tests
mvn clean verify passes
`com.alibaba.fluss.utils.PartitionUtils`
`fluss-client/src/test/java/com/alibaba/fluss/client/table/NewPartitionedTableITCase.java`
### API and Format
This extends the partition name to include more data types.
--
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@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Issue 489 parition key more types apache [fluss]
Posted by "polyzos (via GitHub)" <gi...@apache.org>.
polyzos commented on PR #1264:
URL: https://github.com/apache/fluss/pull/1264#issuecomment-3274946772
and we also need to update the docs accordinly
--
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@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Issue 489 parition key more types apache [fluss]
Posted by "polyzos (via GitHub)" <gi...@apache.org>.
polyzos commented on PR #1264:
URL: https://github.com/apache/fluss/pull/1264#issuecomment-3290952834
@MaggieSullivan thanks you for all the great work 👍 I will merge when the CI passes
--
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@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Issue 489 parition key more types apache [fluss]
Posted by "polyzos (via GitHub)" <gi...@apache.org>.
polyzos commented on PR #1264:
URL: https://github.com/apache/fluss/pull/1264#issuecomment-3034698722
FYI I'm just testing co-pilot today on some PRs, out of curiosity to see how it performs
--
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@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Issue 489 parition key more types apache [fluss]
Posted by "polyzos (via GitHub)" <gi...@apache.org>.
polyzos commented on PR #1264:
URL: https://github.com/apache/fluss/pull/1264#issuecomment-3274925636
@MaggieSullivan can you rebase? And then I will merge, because it looks good
--
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@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Issue 489 parition key more types apache [fluss]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1264:
URL: https://github.com/apache/fluss/pull/1264#discussion_r2184487591
##########
fluss-common/src/main/java/com/alibaba/fluss/utils/PartitionUtils.java:
##########
@@ -134,4 +152,64 @@ public static String generateAutoPartitionTime(
private static String getFormattedTime(ZonedDateTime zonedDateTime, String format) {
return DateTimeFormatter.ofPattern(format).format(zonedDateTime);
}
+
+ public static String convertValueOfType(Object value, DataTypeRoot type) {
Review Comment:
The switch on `type` does not include a default case. If a new or unsupported DataTypeRoot is passed, the method returns an empty string silently. Consider adding a default branch that throws an IllegalArgumentException for unhandled types.
##########
fluss-common/src/test/java/com/alibaba/fluss/utils/PartitionUtilsTest.java:
##########
@@ -148,4 +154,173 @@ void testGenerateAutoPartitionName(
assertThat(resolvedPartitionSpec.getPartitionName()).isEqualTo(expected[i]);
}
}
+
+ @Test
+ void testString() {
+ Object value = BinaryString.fromString("Fluss");
+ DataTypeRoot type = DataTypeRoot.STRING;
+
+ String toStringResult = convertValueOfType(value, type);
+ assertThat(toStringResult).isEqualTo("Fluss");
+ String detectInvalid = detectInvalidName(toStringResult);
+ assertThat(detectInvalid).isEqualTo(null);
+ }
+
+ @Test
+ void testCharConvert() {
+ Object value = BinaryString.fromString("F");
+ DataTypeRoot type = DataTypeRoot.CHAR;
+
+ String toStringResult = convertValueOfType(value, type);
+ assertThat(toStringResult).isEqualTo("F");
+ String detectInvalid = detectInvalidName(toStringResult);
+ assertThat(detectInvalid).isEqualTo(null);
+ }
+
+ @Test
+ void testBooleanConvert() {
+ Object value = true;
+ DataTypeRoot type = DataTypeRoot.BOOLEAN;
+
+ String toStringResult = convertValueOfType(value, type);
+ assertThat(toStringResult).isEqualTo("true");
+ String detectInvalid = detectInvalidName(toStringResult);
+ assertThat(detectInvalid).isEqualTo(null);
+ }
+
+ @Test
+ void testByteConvert() {
+ Object value = new byte[] {0x10, 0x20, 0x30, 0x40, 0x50, (byte) 0b11111111};
+ DataTypeRoot type = DataTypeRoot.BYTES;
+
+ String toStringResult = convertValueOfType(value, type);
+ assertThat(toStringResult).isEqualTo("1020304050ff");
+ String detectInvalid = detectInvalidName(toStringResult);
+ assertThat(detectInvalid).isEqualTo(null);
+ }
+
+ @Test
+ void testBinaryConvert() {
+ Object value = new byte[] {0x10, 0x20, 0x30, 0x40, 0x50, (byte) 0b11111111};
+ DataTypeRoot type = DataTypeRoot.BINARY;
+
+ String toStringResult = convertValueOfType(value, type);
+ assertThat(toStringResult.equals("1020304050ff"));
Review Comment:
This assertion checks a boolean but lacks a terminal assertion (e.g., `isTrue()`). Replace it with `assertThat(toStringResult).isEqualTo("1020304050ff");` for a clear failure message.
```suggestion
assertThat(toStringResult).isEqualTo("1020304050ff");
```
--
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@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Issue 489 parition key more types apache [fluss]
Posted by "polyzos (via GitHub)" <gi...@apache.org>.
polyzos commented on PR #1264:
URL: https://github.com/apache/fluss/pull/1264#issuecomment-3101161470
@wuchong I think this can be merged, if there are no objections
--
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@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Issue 489 parition key more types apache [fluss]
Posted by "MaggieSullivan (via GitHub)" <gi...@apache.org>.
MaggieSullivan commented on PR #1264:
URL: https://github.com/apache/fluss/pull/1264#issuecomment-3290717837
@polyzos I've rebased and updated the documentation. Thank you!
--
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@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Issue 489 parition key more types apache [fluss]
Posted by "polyzos (via GitHub)" <gi...@apache.org>.
polyzos merged PR #1264:
URL: https://github.com/apache/fluss/pull/1264
--
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@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Issue 489 parition key more types apache [fluss]
Posted by "polyzos (via GitHub)" <gi...@apache.org>.
polyzos commented on PR #1264:
URL: https://github.com/apache/fluss/pull/1264#issuecomment-3072281962
I pushed one test, to verify a simple case.. LGTM .. @wuchong can you also take a look? I think we can merge this?
Thank you so much for your contribution @MaggieSullivan
--
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@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Issue 489 parition key more types apache [fluss]
Posted by "MaggieSullivan (via GitHub)" <gi...@apache.org>.
MaggieSullivan commented on PR #1264:
URL: https://github.com/apache/fluss/pull/1264#issuecomment-3284004711
@polyzos Thanks so much. And absolutely, I'll work on your suggestions this weekend.
--
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@fluss.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org