Re: [PR] Add support for `PRINT` statement for SQL Server [datafusion-sqlparser-rs]

2025-04-18 Thread via GitHub 


iffyio merged PR #1811:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1811


-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Add support for `PRINT` statement for SQL Server [datafusion-sqlparser-rs]

2025-04-16 Thread via GitHub 


aharpervc commented on code in PR #1811:
URL: 
https://github.com/apache/datafusion-sqlparser-rs/pull/1811#discussion_r2047549413


##
tests/sqlparser_mssql.rs:
##
@@ -2053,3 +2053,37 @@ fn parse_drop_trigger() {
 }
 );
 }
+
+#[test]
+fn parse_print() {
+let print_string_literal = "PRINT 'Hello, world!'";
+let print_stmt = ms().one_statement_parses_to(print_string_literal, "");
+assert_eq!(
+print_stmt,
+Statement::Print(PrintStatement {
+message: Box::new(Expr::Value(
+(Value::SingleQuotedString("Hello, 
world!".to_string())).with_empty_span()
+)),
+})
+);
+
+let print_national_string = "PRINT N'Hello, ⛄️!'";
+let print_stmt = ms().one_statement_parses_to(print_national_string, "");
+assert_eq!(
+print_stmt,
+Statement::Print(PrintStatement {
+message: Box::new(Expr::Value(
+(Value::NationalStringLiteral("Hello, 
⛄️!".to_string())).with_empty_span()
+)),
+})
+);
+
+let print_variable = "PRINT @my_variable";
+let print_stmt = ms().one_statement_parses_to(print_variable, "");
+assert_eq!(
+print_stmt,
+Statement::Print(PrintStatement {
+message: Box::new(Expr::Identifier(Ident::new("@my_variable"))),
+})
+);

Review Comment:
   I think we might want to inspect the ast in the future if we do an enum of 
value params instead of an expr, but we can cross that bridge if/when we ever 
come to it. This suggestion is complete 👍 



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Add support for `PRINT` statement for SQL Server [datafusion-sqlparser-rs]

2025-04-16 Thread via GitHub 


aharpervc commented on code in PR #1811:
URL: 
https://github.com/apache/datafusion-sqlparser-rs/pull/1811#discussion_r2047240782


##
tests/sqlparser_mssql.rs:
##
@@ -2053,3 +2053,37 @@ fn parse_drop_trigger() {
 }
 );
 }
+
+#[test]
+fn parse_print() {
+let print_string_literal = "PRINT 'Hello, world!'";
+let print_stmt = ms().one_statement_parses_to(print_string_literal, "");
+assert_eq!(
+print_stmt,
+Statement::Print(PrintStatement {
+message: Box::new(Expr::Value(
+(Value::SingleQuotedString("Hello, 
world!".to_string())).with_empty_span()
+)),
+})
+);
+
+let print_national_string = "PRINT N'Hello, ⛄️!'";
+let print_stmt = ms().one_statement_parses_to(print_national_string, "");
+assert_eq!(
+print_stmt,
+Statement::Print(PrintStatement {
+message: Box::new(Expr::Value(
+(Value::NationalStringLiteral("Hello, 
⛄️!".to_string())).with_empty_span()
+)),
+})
+);
+
+let print_variable = "PRINT @my_variable";
+let print_stmt = ms().one_statement_parses_to(print_variable, "");
+assert_eq!(
+print_stmt,
+Statement::Print(PrintStatement {
+message: Box::new(Expr::Identifier(Ident::new("@my_variable"))),
+})
+);

Review Comment:
   I'm unaware of any requirement, it's just that I don't understand the 
project testing conventions & I'm trying to follow the patterns already present 
in the file. I'll switch it over to your suggestion



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Add support for `PRINT` statement for SQL Server [datafusion-sqlparser-rs]

2025-04-15 Thread via GitHub 


iffyio commented on code in PR #1811:
URL: 
https://github.com/apache/datafusion-sqlparser-rs/pull/1811#discussion_r2046111411


##
tests/sqlparser_mssql.rs:
##
@@ -2053,3 +2053,37 @@ fn parse_drop_trigger() {
 }
 );
 }
+
+#[test]
+fn parse_print() {
+let print_string_literal = "PRINT 'Hello, world!'";
+let print_stmt = ms().one_statement_parses_to(print_string_literal, "");
+assert_eq!(
+print_stmt,
+Statement::Print(PrintStatement {
+message: Box::new(Expr::Value(
+(Value::SingleQuotedString("Hello, 
world!".to_string())).with_empty_span()
+)),
+})
+);
+
+let print_national_string = "PRINT N'Hello, ⛄️!'";
+let print_stmt = ms().one_statement_parses_to(print_national_string, "");
+assert_eq!(
+print_stmt,
+Statement::Print(PrintStatement {
+message: Box::new(Expr::Value(
+(Value::NationalStringLiteral("Hello, 
⛄️!".to_string())).with_empty_span()
+)),
+})
+);
+
+let print_variable = "PRINT @my_variable";
+let print_stmt = ms().one_statement_parses_to(print_variable, "");
+assert_eq!(
+print_stmt,
+Statement::Print(PrintStatement {
+message: Box::new(Expr::Identifier(Ident::new("@my_variable"))),
+})
+);

Review Comment:
   oh was there a requirement to use `one_statement_parses_to` instead of 
`verified_stmt` for these tests?
   
   Also with vierified_stmt we can simplify the tests except, for the hello 
world example, to skip assertion on the AST



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Add support for `PRINT` statement for SQL Server [datafusion-sqlparser-rs]

2025-04-15 Thread via GitHub 


aharpervc commented on code in PR #1811:
URL: 
https://github.com/apache/datafusion-sqlparser-rs/pull/1811#discussion_r2044897576


##
src/parser/mod.rs:
##
@@ -617,6 +617,9 @@ impl<'a> Parser<'a> {
 }
 // `COMMENT` is snowflake specific 
https://docs.snowflake.com/en/sql-reference/sql/comment
 Keyword::COMMENT if self.dialect.supports_comment_on() => 
self.parse_comment(),
+Keyword::PRINT if dialect_of!(self is MsSqlDialect) => {

Review Comment:
   Done 👍 



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Add support for `PRINT` statement for SQL Server [datafusion-sqlparser-rs]

2025-04-15 Thread via GitHub 


aharpervc commented on code in PR #1811:
URL: 
https://github.com/apache/datafusion-sqlparser-rs/pull/1811#discussion_r2044896976


##
src/ast/mod.rs:
##
@@ -4050,6 +4050,14 @@ pub enum Statement {
 arguments: Vec,
 options: Vec,
 },
+/// ```sql
+/// PRINT msg_str | @local_variable | string_expr
+/// ```
+///
+/// See: 
https://learn.microsoft.com/en-us/sql/t-sql/statements/print-transact-sql
+Print {
+message: Box,
+},

Review Comment:
   Done 👍 



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Add support for `PRINT` statement for SQL Server [datafusion-sqlparser-rs]

2025-04-15 Thread via GitHub 


aharpervc commented on PR #1811:
URL: 
https://github.com/apache/datafusion-sqlparser-rs/pull/1811#issuecomment-2806527507

   Rebased & updated


-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Add support for `PRINT` statement for SQL Server [datafusion-sqlparser-rs]

2025-04-15 Thread via GitHub 


aharpervc commented on code in PR #1811:
URL: 
https://github.com/apache/datafusion-sqlparser-rs/pull/1811#discussion_r2044896976


##
src/ast/mod.rs:
##
@@ -4050,6 +4050,14 @@ pub enum Statement {
 arguments: Vec,
 options: Vec,
 },
+/// ```sql
+/// PRINT msg_str | @local_variable | string_expr
+/// ```
+///
+/// See: 
https://learn.microsoft.com/en-us/sql/t-sql/statements/print-transact-sql
+Print {
+message: Box,
+},

Review Comment:
   Done



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Add support for `PRINT` statement for SQL Server [datafusion-sqlparser-rs]

2025-04-14 Thread via GitHub 


iffyio commented on code in PR #1811:
URL: 
https://github.com/apache/datafusion-sqlparser-rs/pull/1811#discussion_r2043654919


##
src/parser/mod.rs:
##
@@ -617,6 +617,9 @@ impl<'a> Parser<'a> {
 }
 // `COMMENT` is snowflake specific 
https://docs.snowflake.com/en/sql-reference/sql/comment
 Keyword::COMMENT if self.dialect.supports_comment_on() => 
self.parse_comment(),
+Keyword::PRINT if dialect_of!(self is MsSqlDialect) => {

Review Comment:
   ```suggestion
   Keyword::PRINT => {
   ```
   we can parse unconditionally?



##
src/ast/mod.rs:
##
@@ -4050,6 +4050,14 @@ pub enum Statement {
 arguments: Vec,
 options: Vec,
 },
+/// ```sql
+/// PRINT msg_str | @local_variable | string_expr
+/// ```
+///
+/// See: 
https://learn.microsoft.com/en-us/sql/t-sql/statements/print-transact-sql
+Print {
+message: Box,
+},

Review Comment:
   Yeah we can use a separate struct for repr



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Add support for `PRINT` statement for SQL Server [datafusion-sqlparser-rs]

2025-04-14 Thread via GitHub 


aharpervc commented on code in PR #1811:
URL: 
https://github.com/apache/datafusion-sqlparser-rs/pull/1811#discussion_r2042340044


##
src/ast/mod.rs:
##
@@ -4050,6 +4050,14 @@ pub enum Statement {
 arguments: Vec,
 options: Vec,
 },
+/// ```sql
+/// PRINT msg_str | @local_variable | string_expr
+/// ```
+///
+/// See: 
https://learn.microsoft.com/en-us/sql/t-sql/statements/print-transact-sql
+Print {
+message: Box,
+},

Review Comment:
   Separate struct?



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Add support for `PRINT` statement for SQL Server [datafusion-sqlparser-rs]

2025-04-14 Thread via GitHub 


aharpervc commented on code in PR #1811:
URL: 
https://github.com/apache/datafusion-sqlparser-rs/pull/1811#discussion_r2042341589


##
tests/sqlparser_mssql.rs:
##
@@ -2036,3 +2036,37 @@ fn parse_mssql_merge_with_output() {
 OUTPUT $action, deleted.ProductID INTO dsi.temp_products";
 ms_and_generic().verified_stmt(stmt);
 }
+
+#[test]
+fn parse_print() {

Review Comment:
   Do we need a test that asserts a particular error for a bare `PRINT`? Not 
sure what the conventions are for that



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

[PR] Add support for `PRINT` statement for SQL Server [datafusion-sqlparser-rs]

2025-04-11 Thread via GitHub 


aharpervc opened a new pull request, #1811:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/1811

   Reference: 
https://learn.microsoft.com/en-us/sql/t-sql/language-elements/print-transact-sql?view=sql-server-ver16
   
   Making `message` a `Box` instead of an enum of (national) string 
literal, variable, or expression is _slightly_ lazy and also seems completely 
acceptable for the time being.


-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]