Skip to content
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

MsSQL SET for session params #1646

Merged
merged 2 commits into from
Jan 11, 2025
Merged

Conversation

yoavcloud
Copy link
Contributor

No description provided.

@alamb
Copy link
Contributor

alamb commented Jan 7, 2025

Hi @yoavcloud -- it looks like this PR has some conflicts -- is there any chance you can resolve them?

@yoavcloud yoavcloud force-pushed the mssql_session_params branch from 9e11da8 to ecf5bed Compare January 7, 2025 12:32
@yoavcloud
Copy link
Contributor Author

@alamb done

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Seems good to me -- thanks @yoavcloud -- it would be nice to avoid using Strings quite as much but that is something we could do as a follow on PR if anyone cares

if let Token::Word(w) = next_token.token {
return Ok(w.to_string());
}
self.expected("Session param name", next_token)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe it would be nice to list the expected keywords (IO/XML/PROFILE/TIME, etc) here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are dozens, and I wanted the code to not be dependent on new params being added

}

pub fn parse_set_session_param_name(&mut self) -> Result<String, ParserError> {
if self.parse_keywords(&[Keyword::STATISTICS, Keyword::IO]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Given these are hard coded it seems like it would be cleaner to eventually return an enum for what setting was set rather than arbitrary strings that the using library needs to parse / interpret

However, I also think we could do that as a follow on PR

@alamb
Copy link
Contributor

alamb commented Jan 7, 2025

fyi @iffyio

@yoavcloud yoavcloud force-pushed the mssql_session_params branch from ecf5bed to 36b1a41 Compare January 9, 2025 10:21
@yoavcloud
Copy link
Contributor Author

@alamb @iffyio please see a more structured approach to the parsing of the session parameters

Copy link
Contributor

@iffyio iffyio left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @yoavcloud!

@yoavcloud yoavcloud force-pushed the mssql_session_params branch from 36b1a41 to 3e1743e Compare January 10, 2025 07:25
@yoavcloud yoavcloud force-pushed the mssql_session_params branch from 3e1743e to d2351a9 Compare January 10, 2025 20:18
@iffyio iffyio merged commit 3b4dc0f into apache:main Jan 11, 2025
9 checks passed
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.

3 participants