Skip to content

Conversation

@adelapena
Copy link
Contributor

No description provided.

@adelapena adelapena force-pushed the 17941-trunk branch 5 times, most recently from 979aa1c to 9883c99 Compare November 24, 2022 13:07
Copy link
Contributor

@blerer blerer left a comment

Choose a reason for hiding this comment

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

Thanks for the patch. It is a nice work. The thing that worries me is the fact that some of the functions can change the type on the flight. It is not reflected at the ResultSet metadata level (so the type announced does not match the actual type) and it will also break queries that use selector chaining or that need to take the column type into account. It feels to me like a risky path.
Some how related we should be clear about what masking with null means. Numeric type normally represent null with an empty ByteBuffer, text columns do not support null value. We represent a missing column value through null is it what we want? It should not work for primary key components in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: The javadoc of 'getOrCreateFunction' should specify that the method can return null value. The annotation is here but it would better to have it specified in the the javadoc. I read the doc but did not pay attention to the annotation so I guess that others could do the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, added a comment on the JavaDoc for getOrCreateFunction, and also in the one for the internal doGetOrCreateFunction.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking of selector chaining. Is it safe to change the column type on the flight? Do we also change the Resultset metadata?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On regular queries we do change the ResultSet metadata, I have added some specific testing for this. Selector chaining should work as with using any other function. That might be more tricky when we attach functions to columns at CASSANDRA-18068, and we probably should forbid attaching this kind of type-altering functions. But using them as regular functions on SELECT queries shouldn't be a problem.

@adelapena
Copy link
Contributor Author

Thanks for the review. I have rebased the branch and tried to address the suggestions on an additional commit. CI:

As discussed on Slack, the proposed functions are taken into account in the ResultSet metadata when the functions are used as regular function calls in a SELECT query. That metadata will be comprised by functions calls with the adequate type. Thus, using functions that return a data type different to their input shouldn't be a problem when directly using the functions in queries, like for example SELECT mask_hash(v) FROM t. In that example, the ResultSet metadata will contain a column named mask_hash(v) of type blob. That shouldn't surprise the user running the query.

However, we should take those return types into account during CASSANDRA-18068. On that ticket we will allow to attach the utility masking functions to table columns on CREATE/ALTER TABLE. That can be problematic because in that case the user query is not explicitly invoking the function, and the name and type of the column don't change on the ResultSet.

Probably we will need to just forbid attaching type-altering masking functions to column definitions on CREATE/ALTER TABLE. We could also forbid them by default but have a config property that allows that kind of mapping, if we see any value to it. However, there doesn't seem to be a reason to forbid the explicit use of those functions on regular SELECT queries.

@adelapena adelapena force-pushed the 17941-trunk branch 2 times, most recently from 9c830c3 to 6a9b654 Compare January 24, 2023 13:48
patch by Andrés de la Peña; reviewed by Benjamin Lerer for CASSANDRA-17941
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants