-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: new syntax to interact with session variables (define/undefine/show variables) #6474
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@@ -566,6 +581,10 @@ TIMESTAMP_WITH_TIME_ZONE | |||
: 'TIMESTAMP' WS 'WITH' WS 'TIME' WS 'ZONE' | |||
; | |||
|
|||
VARIABLE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this used anywhere for this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No in this PR. I used it initially, but after splitting the PR I left it by accident.
I will leave it here. The next PR will use it anyway, so doesn't hurt.
@RunWith(MockitoJUnitRunner.class) | ||
public class ListVariablesExecutorTest { | ||
@Rule | ||
public final TemporaryEngine engine = new TemporaryEngine(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you need to actually run through configure etc... or can you mock out the configured statements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied most of the code from ListTopicsExecutorTest and ListPropertiesExecutorTest. But you're right, we don't need this engine. I'll change it with a mock.
} | ||
|
||
@Test | ||
public void shouldThrowOnInvalidValues() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we add a test on adding duplicates? we should also check to make sure that case insensitive duplicates are stilled rejected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variables will be overridden (no rejected) if an existing case insensitive variable exists. I'll add a test case that verifies that.
It will allow us to do this:
DEFINE var1 = '1';
DEFINE VAR1 = '2';
DEFINE vAr1 = '3'; // latest update
ac1b0df
to
ec1c89d
Compare
ec1c89d
to
a9a9ba8
Compare
Description
This is part of KLIP-38 - Variable Substitution
Variable substitution will have a follow-up PR.
It adds a syntax to interact with session variables:
^ The KLIP proposed the use of
DEFINE
to list values. But I saw theSHOW
more appropriate as we have SHOW and LIST syntax for other entities. I will change the KLIP after variable substitution is merged.The syntax is added on the server-side and CLI-side.
For the server-side, the variable scope is seen per-request. i.e.
For the CLI-side, the variable scope is during the session of the CLI. i.e.
Testing done
Added unit tests
Verified manually (see above examples)
Reviewer checklist