CQL Function Handling#176
Conversation
brandtnewton
force-pushed
the
proxy/function-refactor
branch
from
39ec6c7 to
8da6a73
Compare
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades the Cassandra-Bigtable proxy's ability to process and translate CQL queries involving various built-in functions. The changes introduce a robust framework for handling functions like Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-structured refactoring for handling CQL functions. The introduction of the CqlFunc interface and the DynamicValue hierarchy greatly improves the modularity and extensibility of function handling. The changes are extensive but follow a clear pattern, moving towards a more object-oriented design.
However, I've found several bugs in the new function implementations, particularly for minTimeuuid, maxTimeuuid, and toTimestamp, as well as a logic error in FlipOperator. I've also noted an incorrect test assertion change. These issues should be addressed to ensure correctness.
| u[8] = 0x7f | ||
|
|
||
| // clock_seq_low (byte 9, 8 bits) | ||
| u[9] = 0x7f | ||
|
|
||
| // Node ID (bytes 10-15, 48 bits) - set to all F's (max) | ||
| u[10] = 0x7f | ||
| u[11] = 0x7f | ||
| u[12] = 0x7f | ||
| u[13] = 0x7f | ||
| u[14] = 0x7f | ||
| u[15] = 0x7f |
There was a problem hiding this comment.
The implementation for generating the maximum timeuuid is incorrect. To get the maximum UUIDv1 for a given time, the clock sequence and node ID fields should be set to their maximum values (0xFF), and the variant bits in u[8] must be set correctly.
Specifically:
u[8]should be0xBFto represent the variant10and have the clock sequence high bits maxed out.u[9](clock sequence low) should be0xFF.u[10]throughu[15](node ID) should also be0xFF.
The current implementation uses 0x7f, which is incorrect and doesn't set the variant bits properly.
| u[8] = 0x7f | |
| // clock_seq_low (byte 9, 8 bits) | |
| u[9] = 0x7f | |
| // Node ID (bytes 10-15, 48 bits) - set to all F's (max) | |
| u[10] = 0x7f | |
| u[11] = 0x7f | |
| u[12] = 0x7f | |
| u[13] = 0x7f | |
| u[14] = 0x7f | |
| u[15] = 0x7f | |
| u[8] = 0xBF | |
| // clock_seq_low (byte 9, 8 bits) | |
| u[9] = 0xFF | |
| // Node ID (bytes 10-15, 48 bits) - set to all F's (max) | |
| u[10] = 0xFF | |
| u[11] = 0xFF | |
| u[12] = 0xFF | |
| u[13] = 0xFF | |
| u[14] = 0xFF | |
| u[15] = 0xFF |
| u[9] = 0x80 // Min 8 bits | ||
|
|
||
| // Node ID (bytes 10-15, 48 bits) - set to all 0's (min) | ||
| u[10] = 0x80 | ||
| u[11] = 0x80 | ||
| u[12] = 0x80 | ||
| u[13] = 0x80 | ||
| u[14] = 0x80 | ||
| u[15] = 0x80 |
There was a problem hiding this comment.
The implementation for generating the minimum timeuuid is incorrect. To get the minimum UUIDv1 for a given time, the clock sequence and node ID fields should be set to their minimum values (0x00).
Specifically:
u[9](clock sequence low) should be0x00.u[10]throughu[15](node ID) should also be0x00.
The current implementation uses 0x80 for these bytes, which is not the minimum value.
| u[9] = 0x80 // Min 8 bits | |
| // Node ID (bytes 10-15, 48 bits) - set to all 0's (min) | |
| u[10] = 0x80 | |
| u[11] = 0x80 | |
| u[12] = 0x80 | |
| u[13] = 0x80 | |
| u[14] = 0x80 | |
| u[15] = 0x80 | |
| u[9] = 0x00 // Min 8 bits | |
| // Node ID (bytes 10-15, 48 bits) - set to all 0's (min) | |
| u[10] = 0x00 | |
| u[11] = 0x00 | |
| u[12] = 0x00 | |
| u[13] = 0x00 | |
| u[14] = 0x00 | |
| u[15] = 0x00 |
| if dt.Code() == INT || dt.Code() == BIGINT { | ||
| return TypeFloat | ||
| } |
There was a problem hiding this comment.
According to Cassandra documentation, the avg function on int or bigint columns returns a varint. This implementation returns TypeFloat. While this might be a deliberate simplification, it deviates from the standard Cassandra behavior. Consider returning TypeVarint for better compatibility.
| if dt.Code() == INT || dt.Code() == BIGINT { | |
| return TypeFloat | |
| } | |
| if dt.Code() == INT || dt.Code() == BIGINT { | |
| return TypeVarint | |
| } |
| case GTE: | ||
| return LTE | ||
| case LTE: | ||
| return LTE |
There was a problem hiding this comment.
There's a logic error here. Flipping the <= (LTE) operator should result in >= (GTE), but it's currently returning LTE.
| return LTE | |
| return GTE |
| assert.Equal(t, &t1, got.id) | ||
| assert.Equal(t, "3", got.val) | ||
| assert.Equal(t, &t2, got.parentEvent) |
There was a problem hiding this comment.
These assertions are comparing pointers instead of the values they point to. This will likely not behave as expected. You should dereference the pointers before comparing with the expected UUID values.
| assert.Equal(t, &t1, got.id) | |
| assert.Equal(t, "3", got.val) | |
| assert.Equal(t, &t2, got.parentEvent) | |
| assert.Equal(t, t1, *got.id) | |
| assert.Equal(t, "3", got.val) | |
| assert.Equal(t, t2, *got.parentEvent) |
| assert.Equal(t, &t1, got.id) | ||
| assert.Equal(t, "3", got.val) | ||
| assert.Equal(t, &t2, got.parentEvent) |
There was a problem hiding this comment.
Similar to the above, these assertions are comparing pointers instead of values. Please dereference got.id and got.parentEvent for a correct value comparison.
| assert.Equal(t, &t1, got.id) | |
| assert.Equal(t, "3", got.val) | |
| assert.Equal(t, &t2, got.parentEvent) | |
| assert.Equal(t, t1, *got.id) | |
| assert.Equal(t, "3", got.val) | |
| assert.Equal(t, t2, *got.parentEvent) |
1 participant
Adds proper support for CQL function handling: minTimeuuid, maxTimeuuid, etc.