Support Cassandra nested collection type #657
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ebyhr
force-pushed
the
cassandra/nested-collection-v2
branch
from
563b6c0
to
2709120
Compare
|
Let me check travis failure. |
ebyhr
force-pushed
the
cassandra/nested-collection-v2
branch
from
2709120
to
88f9138
Compare
|
I assume the travis failure (https://api.travis-ci.com/v3/job/194830803/log.txt) isn't related to this PR. |
findepi
reviewed
Apr 23, 2019
| private static String buildArrayValue(Row row, int position) | ||
| { | ||
| Definition definition = row.getColumnDefinitions().asList().get(position); | ||
| DataType elemType = definition.getType().getTypeArguments().get(0); |
There was a problem hiding this comment.
- don't abbreviate: elemType -> elementType
- use
com.google.common.collect.Iterables#getOnlyElement
Suggested change
| DataType elemType = definition.getType().getTypeArguments().get(0); | |
| DataType elementType = getOnlyElement(definition.getType().getTypeArguments()); |
| { | ||
| return buildArrayValue(row.getSet(position, elemType.javaType), elemType); | ||
| Definition definition = row.getColumnDefinitions().asList().get(position); | ||
| DataType keyType = definition.getType().getTypeArguments().get(0); |
There was a problem hiding this comment.
checkArgument(definition.getType().getTypeArguments().size() == 2, "Expected two type arguments, got: %s", definition.getType().getTypeArguments())
| } | ||
|
|
||
| private static String buildMapValue(Row row, int position, CassandraType keyType, CassandraType valueType) | ||
| private static String buildMapValue(Map<?, ?> collection, DataType keyType, DataType valueType) |
| @@ -383,32 +356,13 @@ private static String objectToString(Object object, CassandraType elemType) | |||
| case FLOAT: | |||
| case DECIMAL: | |||
| return object.toString(); | |||
| case LIST: | |||
| case SET: | |||
| return buildArrayValue((Collection<?>) object, dataType.getTypeArguments().get(0)); | |||
There was a problem hiding this comment.
Suggested change
| return buildArrayValue((Collection<?>) object, dataType.getTypeArguments().get(0)); | |
| return buildArrayValue((Collection<?>) object, getOnlyElement(dataType.getTypeArguments())); |
| { | ||
| return buildArrayValue(row.getSet(position, elemType.javaType), elemType); | ||
| Definition definition = row.getColumnDefinitions().asList().get(position); |
There was a problem hiding this comment.
You can replace .asList().get(position).getType() with .getType(position)
Suggested change
| Definition definition = row.getColumnDefinitions().asList().get(position); | |
| Type type = row.getColumnDefinitions().getType(position); |
| @@ -520,6 +474,22 @@ public Object validateClusteringKey(Object value) | |||
| } | |||
| } | |||
|
|
|||
| public static boolean containsUnsupportedType(DataType dataType) | |||
| { | |||
| if (!dataType.isCollection()) { | |||
There was a problem hiding this comment.
This could probably be simplified to (i also inverted meaning of the method):
public static boolean isFullySupported(DataType dataType)
{
if (!toCassandraType(dataType.getName()).isPresent()) {
return false;
}
return dataType.getTypeArguments().stream()
.allMatch(CassandraType::isFullySupported);
}
| import static org.testng.Assert.assertTrue; | ||
|
|
||
| public class TestCassandraType | ||
| { | ||
| @Test | ||
| public void testContainsUnsupportedType() |
There was a problem hiding this comment.
I wouldn't test io.prestosql.plugin.cassandra.CassandraType#containsUnsupportedType, for 2 reasons
- as we add more supported types, we will need to update the test
- the method is not that complex in itself (simple recursive visit over the type term), it's more about the assumption this method makes, how it interacts with other parts of the code base.
this is my opinion, though. Let me know if you disagree.
...cassandra/src/test/java/io/prestosql/plugin/cassandra/TestCassandraIntegrationSmokeTest.java
Show resolved
Hide resolved
| @@ -428,6 +428,38 @@ public void testUnsupportedColumnType() | |||
| session.execute("DROP KEYSPACE keyspace_6"); | |||
| } | |||
|
|
|||
| public void testNestedCollectionType() | |||
| { | |||
| session.execute("CREATE KEYSPACE keyspace_7 WITH REPLICATION = {'class':'SimpleStrategy', 'replication_factor': 1}"); | |||
There was a problem hiding this comment.
Can we keyspace_7 -> keyspace_test_nested_collection (or other meaningful name)?
...cassandra/src/test/java/io/prestosql/plugin/cassandra/TestCassandraIntegrationSmokeTest.java
Show resolved
Hide resolved
ebyhr
force-pushed
the
cassandra/nested-collection-v2
branch
from
88f9138
to
4ec5458
Compare
|
@findepi Updated and confirmed failed travis isn't related to this PR. |
|
Merged, thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #655
Current CassandraType can't handle nested collection type. This PR support the type and remove
FullCassandraTypeinterface to simplify type mapping.