You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "lowka (via GitHub)" <gi...@apache.org> on 2024/07/25 14:37:10 UTC
[PR] IGNITE-22827: Sql. Delete ColumnType / NativeTypeSpec BITMASK, NUMBER [ignite-3]
lowka opened a new pull request, #4141:
URL: https://github.com/apache/ignite-3/pull/4141
Deletes `BITMASK`, `NUMBER` types and related methods.
---
Thank you for submitting the pull request.
To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:
### The Review Checklist
- [ ] **Formal criteria:** TC status, codestyle, mandatory documentation. Also make sure to complete the following:
\- There is a single JIRA ticket related to the pull request.
\- The web-link to the pull request is attached to the JIRA ticket.
\- The JIRA ticket has the Patch Available state.
\- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
\- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
- [ ] **Design:** new code conforms with the design principles of the components it is added to.
- [ ] **Patch quality:** patch cannot be split into smaller pieces, its size must be reasonable.
- [ ] **Code quality:** code is clean and readable, necessary developer documentation is added if needed.
- [ ] **Tests code quality:** test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.
### Notes
- [Apache Ignite Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Java+Code+Style+Guide)
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-22827: Sql. Delete ColumnType / NativeTypeSpec BITMASK, NUMBER [ignite-3]
Posted by "lowka (via GitHub)" <gi...@apache.org>.
lowka commented on code in PR #4141:
URL: https://github.com/apache/ignite-3/pull/4141#discussion_r1698093485
##########
modules/api/src/main/java/org/apache/ignite/sql/ColumnType.java:
##########
@@ -106,12 +100,12 @@ public enum ColumnType {
private final int id;
- private static final ColumnType[] VALS = new ColumnType[values().length];
+ private static final Int2ObjectMap<ColumnType> VALS = new Int2ObjectOpenHashMap<>();
Review Comment:
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-22827: Sql. Delete ColumnType / NativeTypeSpec BITMASK, NUMBER [ignite-3]
Posted by "lowka (via GitHub)" <gi...@apache.org>.
lowka commented on code in PR #4141:
URL: https://github.com/apache/ignite-3/pull/4141#discussion_r1696993046
##########
modules/system-view/src/test/java/org/apache/ignite/internal/systemview/SystemViewManagerTest.java:
##########
@@ -165,8 +165,7 @@ public void registerAllColumnTypes(NativeTypeSpec typeSpec) {
}
private static Stream<NativeTypeSpec> nativeTypeSpecs() {
- return Stream.of(NativeTypeSpec.values())
- .filter(t -> t != NativeTypeSpec.BITMASK && t != NativeTypeSpec.NUMBER);
+ return Stream.of(NativeTypeSpec.values());
Review Comment:
Fixed.
##########
modules/schema/src/test/java/org/apache/ignite/internal/schema/catalog/CatalogToSchemaDescriptorConverterTest.java:
##########
@@ -57,8 +57,7 @@ public class CatalogToSchemaDescriptorConverterTest extends AbstractSchemaConver
private static final int TEST_SCALE = 5;
private static Stream<NativeTypeSpec> nativeTypeSpecs() {
- return Stream.of(NativeTypeSpec.values())
- .filter(t -> t != NativeTypeSpec.NUMBER && t != NativeTypeSpec.BITMASK);
+ return Stream.of(NativeTypeSpec.values());
Review Comment:
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-22827: Sql. Delete ColumnType / NativeTypeSpec BITMASK, NUMBER [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on PR #4141:
URL: https://github.com/apache/ignite-3/pull/4141#issuecomment-2258501901
Looks good, but two things with the remaining TODOs in the code worry me.
I would suggest doing the following:
1. ItDynamicParameterTest
Change comment
```
// https://issues.apache.org/jira/browse/IGNITE-18414
```
to
```
// https://issues.apache.org/jira/browse/IGNITE-15200
```
2. In class PlatformTestNodeRunner change comment
```
// TODO IGNITE-18431 remove extra table, use TABLE_NAME_ALL_COLUMNS for SQL tests.
```
To
```
// TODO IGNITE-22856 use TABLE_NAME_ALL_COLUMNS for SQL tests, remove this table if it is no longer used
// TODO IGNITE-22857 use TABLE_NAME_ALL_COLUMNS for SQL tests, remove this table if it is no longer used
```
(one for .NET and one for ODBC)
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-22827: Sql. Delete ColumnType / NativeTypeSpec BITMASK, NUMBER [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #4141:
URL: https://github.com/apache/ignite-3/pull/4141#discussion_r1697050007
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/row/RowSchemaTypesTest.java:
##########
@@ -124,10 +124,7 @@ public Stream<DynamicTest> testFromNativeTypeWithNullability() {
Set<Entry<NativeTypeSpec, Boolean>> nativeTypeSpecs = new CopyOnWriteArraySet<>();
- EnumSet<NativeTypeSpec> set = EnumSet.allOf(NativeTypeSpec.class);
- set.removeAll(Set.of(NativeTypeSpec.BITMASK, NativeTypeSpec.NUMBER));
-
- for (NativeTypeSpec spec : set) {
+ for (NativeTypeSpec spec : EnumSet.allOf(NativeTypeSpec.class)) {
Review Comment:
```suggestion
for (NativeTypeSpec spec : NativeTypeSpec.values()) {
```
(remove unused import, after such a change)
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-22827: Sql. Delete ColumnType / NativeTypeSpec BITMASK, NUMBER [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich merged PR #4141:
URL: https://github.com/apache/ignite-3/pull/4141
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-22827: Sql. Delete ColumnType / NativeTypeSpec BITMASK, NUMBER [ignite-3]
Posted by "lowka (via GitHub)" <gi...@apache.org>.
lowka commented on code in PR #4141:
URL: https://github.com/apache/ignite-3/pull/4141#discussion_r1698116252
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/row/RowSchemaTypesTest.java:
##########
@@ -124,10 +124,7 @@ public Stream<DynamicTest> testFromNativeTypeWithNullability() {
Set<Entry<NativeTypeSpec, Boolean>> nativeTypeSpecs = new CopyOnWriteArraySet<>();
- EnumSet<NativeTypeSpec> set = EnumSet.allOf(NativeTypeSpec.class);
- set.removeAll(Set.of(NativeTypeSpec.BITMASK, NativeTypeSpec.NUMBER));
-
- for (NativeTypeSpec spec : set) {
+ for (NativeTypeSpec spec : EnumSet.allOf(NativeTypeSpec.class)) {
Review Comment:
Regarding the IGNITE-18431. I think that this issue is not resolved and we should keep it.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-22827: Sql. Delete ColumnType / NativeTypeSpec BITMASK, NUMBER [ignite-3]
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #4141:
URL: https://github.com/apache/ignite-3/pull/4141#discussion_r1697995758
##########
modules/api/src/main/java/org/apache/ignite/sql/ColumnType.java:
##########
@@ -106,12 +100,12 @@ public enum ColumnType {
private final int id;
- private static final ColumnType[] VALS = new ColumnType[values().length];
+ private static final Int2ObjectMap<ColumnType> VALS = new Int2ObjectOpenHashMap<>();
Review Comment:
I'm wondering why map is better than array? we have only one gap, and the only method using this collections is ready to `null` being returned. Does it make sense to just mark values in the array as `@Nullable`
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-22827: Sql. Delete ColumnType / NativeTypeSpec BITMASK, NUMBER [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #4141:
URL: https://github.com/apache/ignite-3/pull/4141#discussion_r1696841436
##########
modules/system-view/src/test/java/org/apache/ignite/internal/systemview/SystemViewManagerTest.java:
##########
@@ -165,8 +165,7 @@ public void registerAllColumnTypes(NativeTypeSpec typeSpec) {
}
private static Stream<NativeTypeSpec> nativeTypeSpecs() {
- return Stream.of(NativeTypeSpec.values())
- .filter(t -> t != NativeTypeSpec.BITMASK && t != NativeTypeSpec.NUMBER);
+ return Stream.of(NativeTypeSpec.values());
Review Comment:
EnumSource
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-22827: Sql. Delete ColumnType / NativeTypeSpec BITMASK, NUMBER [ignite-3]
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #4141:
URL: https://github.com/apache/ignite-3/pull/4141#discussion_r1696834755
##########
modules/schema/src/test/java/org/apache/ignite/internal/schema/catalog/CatalogToSchemaDescriptorConverterTest.java:
##########
@@ -57,8 +57,7 @@ public class CatalogToSchemaDescriptorConverterTest extends AbstractSchemaConver
private static final int TEST_SCALE = 5;
private static Stream<NativeTypeSpec> nativeTypeSpecs() {
- return Stream.of(NativeTypeSpec.values())
- .filter(t -> t != NativeTypeSpec.NUMBER && t != NativeTypeSpec.BITMASK);
+ return Stream.of(NativeTypeSpec.values());
Review Comment:
it will be better to rewrite it to EnumSource instead of the method
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-22827: Sql. Delete ColumnType / NativeTypeSpec BITMASK, NUMBER [ignite-3]
Posted by "lowka (via GitHub)" <gi...@apache.org>.
lowka commented on code in PR #4141:
URL: https://github.com/apache/ignite-3/pull/4141#discussion_r1698093783
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/row/RowSchemaTypesTest.java:
##########
@@ -124,10 +124,7 @@ public Stream<DynamicTest> testFromNativeTypeWithNullability() {
Set<Entry<NativeTypeSpec, Boolean>> nativeTypeSpecs = new CopyOnWriteArraySet<>();
- EnumSet<NativeTypeSpec> set = EnumSet.allOf(NativeTypeSpec.class);
- set.removeAll(Set.of(NativeTypeSpec.BITMASK, NativeTypeSpec.NUMBER));
-
- for (NativeTypeSpec spec : set) {
+ for (NativeTypeSpec spec : EnumSet.allOf(NativeTypeSpec.class)) {
Review Comment:
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org