You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "adelapena (via GitHub)" <gi...@apache.org> on 2023/09/27 20:58:26 UTC
[GitHub] [cassandra] adelapena opened a new pull request, #2747: CASSANDRA-18889 5.0: Fix vector type to support DDM's mask_default function
adelapena opened a new pull request, #2747:
URL: https://github.com/apache/cassandra/pull/2747
(no comment)
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18889 5.0: Fix vector type to support DDM's mask_default function [cassandra]
Posted by "pmcfadin (via GitHub)" <gi...@apache.org>.
pmcfadin closed pull request #2747: CASSANDRA-18889 5.0: Fix vector type to support DDM's mask_default function
URL: https://github.com/apache/cassandra/pull/2747
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] Maxwell-Guo commented on a diff in pull request #2747: CASSANDRA-18889 5.0: Fix vector type to support DDM's mask_default function
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2747:
URL: https://github.com/apache/cassandra/pull/2747#discussion_r1339476174
##########
test/unit/org/apache/cassandra/cql3/functions/masking/ColumnMaskQueryWithDefaultTest.java:
##########
@@ -39,7 +42,11 @@ public static Collection<Object[]> options()
options.add(new Object[]{ order, "DEFAULT", "text", "abc", "****" });
options.add(new Object[]{ order, "DEFAULT", "int", 123, 0 });
options.add(new Object[]{ order, "mask_default()", "text", "abc", "****" });
- options.add(new Object[]{ order, "mask_default()", "int", 123, 0, });
+ options.add(new Object[]{ order, "mask_default()", "int", 123, 0 });
+ // TODO: the driver version that we use doesn't support vectors, so we have to use raw values by now
+ options.add(new Object[]{ order, "mask_default()", "vector<int, 2>",
Review Comment:
Is it necessary to add any other element type's case like float ?
##########
src/java/org/apache/cassandra/db/marshal/VectorType.java:
##########
@@ -359,6 +365,13 @@ private <V> void checkConsumedFully(V buffer, ValueAccessor<V> accessor, int off
throw new MarshalException("Unexpected " + remaining + " extraneous bytes after " + asCQL3Type() + " value");
}
+ @Override
+ public ByteBuffer getMaskedValue()
+ {
+ List<ByteBuffer> values = Collections.nCopies(dimension, elementType.getMaskedValue());
Review Comment:
Do we need to add a doc to tell the users how vectortype's mask_default was implemented ? let users know whether the usage expectations are met.
##########
test/unit/org/apache/cassandra/cql3/functions/masking/ColumnMaskQueryWithReplaceTest.java:
##########
@@ -43,6 +46,10 @@ public static Collection<Object[]> options()
options.add(new Object[]{ order, "mask_replace(0)", "int", 123, 0 });
options.add(new Object[]{ order, "mask_replace(0)", "bigint", 123L, 0L });
options.add(new Object[]{ order, "mask_replace(0)", "varint", BigInteger.valueOf(123), BigInteger.ZERO });
+ // TODO: the driver version that we use doesn't support vectors, so we have to use raw values by now
+ options.add(new Object[]{ order, "mask_replace([0, 0])", "vector<int, 2>",
Review Comment:
same with the upper case
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] adelapena commented on a diff in pull request #2747: CASSANDRA-18889 5.0: Fix vector type to support DDM's mask_default function
Posted by "adelapena (via GitHub)" <gi...@apache.org>.
adelapena commented on code in PR #2747:
URL: https://github.com/apache/cassandra/pull/2747#discussion_r1340381632
##########
src/java/org/apache/cassandra/db/marshal/VectorType.java:
##########
@@ -359,6 +365,13 @@ private <V> void checkConsumedFully(V buffer, ValueAccessor<V> accessor, int off
throw new MarshalException("Unexpected " + remaining + " extraneous bytes after " + asCQL3Type() + " value");
}
+ @Override
+ public ByteBuffer getMaskedValue()
+ {
+ List<ByteBuffer> values = Collections.nCopies(dimension, elementType.getMaskedValue());
Review Comment:
I have added some doc lines and examples about how default masking values are generated.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18889 5.0: Fix vector type to support DDM's mask_default function [cassandra]
Posted by "adelapena (via GitHub)" <gi...@apache.org>.
adelapena commented on code in PR #2747:
URL: https://github.com/apache/cassandra/pull/2747#discussion_r1346186729
##########
src/java/org/apache/cassandra/cql3/CQL3Type.java:
##########
@@ -566,6 +567,21 @@ public String toCQLLiteral(ByteBuffer buffer)
return sb.toString();
}
+ @Override
Review Comment:
Yes, they should. Without proper hashcode/equals we can end up seeing messages such as `Masking function mask_default() return type is vector<int, 2>. This is different to the type of the masked column r2 of type vector<int, 2>.`
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18889 5.0: Fix vector type to support DDM's mask_default function [cassandra]
Posted by "bereng (via GitHub)" <gi...@apache.org>.
bereng commented on code in PR #2747:
URL: https://github.com/apache/cassandra/pull/2747#discussion_r1343646723
##########
src/java/org/apache/cassandra/cql3/CQL3Type.java:
##########
@@ -566,6 +567,21 @@ public String toCQLLiteral(ByteBuffer buffer)
return sb.toString();
}
+ @Override
Review Comment:
These should have been added when Vector was added correct?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] adelapena commented on a diff in pull request #2747: CASSANDRA-18889 5.0: Fix vector type to support DDM's mask_default function
Posted by "adelapena (via GitHub)" <gi...@apache.org>.
adelapena commented on code in PR #2747:
URL: https://github.com/apache/cassandra/pull/2747#discussion_r1340395549
##########
doc/modules/cassandra/examples/CQL/ddm_create_table_with_udf.cql:
##########
@@ -2,7 +2,7 @@ CREATE FUNCTION redact(input text)
CALLED ON NULL INPUT
RETURNS text
LANGUAGE java
- AS 'return "redacted";
+ AS 'return "redacted";';
Review Comment:
This has been kindly found by @sarma1807 . I'm piggy backing this little fix here since it probably isn't worth a full ticket.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18889 5.0: Fix vector type to support DDM's mask_default function [cassandra]
Posted by "pmcfadin (via GitHub)" <gi...@apache.org>.
pmcfadin commented on PR #2747:
URL: https://github.com/apache/cassandra/pull/2747#issuecomment-4227140263
Closing as [CASSANDRA-18889](https://issues.apache.org/jira/browse/CASSANDRA-18889) has been resolved as Fixed — if this PR contains changes not captured in the merged fix, please reopen.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18889 5.0: Fix vector type to support DDM's mask_default function [cassandra]
Posted by "bereng (via GitHub)" <gi...@apache.org>.
bereng commented on code in PR #2747:
URL: https://github.com/apache/cassandra/pull/2747#discussion_r1343646723
##########
src/java/org/apache/cassandra/cql3/CQL3Type.java:
##########
@@ -566,6 +567,21 @@ public String toCQLLiteral(ByteBuffer buffer)
return sb.toString();
}
+ @Override
Review Comment:
This should have been added when Vector was added correct?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] adelapena commented on a diff in pull request #2747: CASSANDRA-18889 5.0: Fix vector type to support DDM's mask_default function
Posted by "adelapena (via GitHub)" <gi...@apache.org>.
adelapena commented on code in PR #2747:
URL: https://github.com/apache/cassandra/pull/2747#discussion_r1340390917
##########
test/unit/org/apache/cassandra/cql3/functions/masking/ColumnMaskQueryWithDefaultTest.java:
##########
@@ -39,7 +42,11 @@ public static Collection<Object[]> options()
options.add(new Object[]{ order, "DEFAULT", "text", "abc", "****" });
options.add(new Object[]{ order, "DEFAULT", "int", 123, 0 });
options.add(new Object[]{ order, "mask_default()", "text", "abc", "****" });
- options.add(new Object[]{ order, "mask_default()", "int", 123, 0, });
+ options.add(new Object[]{ order, "mask_default()", "int", 123, 0 });
+ // TODO: the driver version that we use doesn't support vectors, so we have to use raw values by now
+ options.add(new Object[]{ order, "mask_default()", "vector<int, 2>",
Review Comment:
These tests focus on how different queries interact with masking rather than on what each particular function and data type combination does. It's useful to test different functions just in case, but we cannot add too many combinations here because each function/type variation multiplies the number of parameterized cases, making the test slower. That's why we only test with a single function and a few data types. At the moment the test takes 2-3 minutes to run, and adding more cases might make it timeout on CI.
Instead, I have added a specific randomized/fuzz test on `AbstractTypeTest` that would cover many data type combinations.
##########
test/unit/org/apache/cassandra/cql3/functions/masking/ColumnMaskQueryWithReplaceTest.java:
##########
@@ -43,6 +46,10 @@ public static Collection<Object[]> options()
options.add(new Object[]{ order, "mask_replace(0)", "int", 123, 0 });
options.add(new Object[]{ order, "mask_replace(0)", "bigint", 123L, 0L });
options.add(new Object[]{ order, "mask_replace(0)", "varint", BigInteger.valueOf(123), BigInteger.ZERO });
+ // TODO: the driver version that we use doesn't support vectors, so we have to use raw values by now
+ options.add(new Object[]{ order, "mask_replace([0, 0])", "vector<int, 2>",
Review Comment:
Same as above.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18889 5.0: Fix vector type to support DDM's mask_default function [cassandra]
Posted by "bereng (via GitHub)" <gi...@apache.org>.
bereng commented on PR #2747:
URL: https://github.com/apache/cassandra/pull/2747#issuecomment-1748343688
+1
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] sarma1807 commented on a diff in pull request #2747: CASSANDRA-18889 5.0: Fix vector type to support DDM's mask_default function
Posted by "sarma1807 (via GitHub)" <gi...@apache.org>.
sarma1807 commented on code in PR #2747:
URL: https://github.com/apache/cassandra/pull/2747#discussion_r1341878178
##########
doc/modules/cassandra/examples/CQL/ddm_create_table_with_udf.cql:
##########
@@ -2,7 +2,7 @@ CREATE FUNCTION redact(input text)
CALLED ON NULL INPUT
RETURNS text
LANGUAGE java
- AS 'return "redacted";
+ AS 'return "redacted";';
Review Comment:
Thank you very much for the fix @adelapena
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org