[GitHub] [flink] snuyanzin commented on pull request #22624: [FLINK-32132][table-planner] Cast function CODEGEN does not work as e…

2023-07-03 Thread via GitHub 


snuyanzin commented on PR #22624:
URL: https://github.com/apache/flink/pull/22624#issuecomment-1618312678

   @zhougit86 thanks for the contribution
   are you willing to create backports to 1.17.x, 1.16.x?


-- 
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: issues-unsubscr...@flink.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [flink] snuyanzin commented on pull request #22624: [FLINK-32132][table-planner] Cast function CODEGEN does not work as e…

2023-06-28 Thread via GitHub 


snuyanzin commented on PR #22624:
URL: https://github.com/apache/flink/pull/22624#issuecomment-1612073732

   @zhougit86 sorry for the late response
   
   I have a question: could you please clarify it
   it seems your change is inside condition
   ```scala
   if (codeGeneratorCastRule.canFail(inputType, targetType) && nullOnFailure) {
   ...
   val adjustedTargetType = if (nullOnFailure) targetType.copy(nullOnFailure) 
else targetType
 return GeneratedExpression(
   resultTerm,
   nullTerm,
   operand.code + "\n" + castCode,
   adjustedTargetType
 )
   ...
   } else {
   ...
 return GeneratedExpression(
   castCodeBlock.getReturnTerm,
   castCodeBlock.getIsNullTerm,
   operand.code + castCode,
   targetType
 )
   }
   ```
   shouldn't we extract `val adjustedTargetType = if (nullOnFailure) 
targetType.copy(nullOnFailure) else targetType` from `if/else` and apply it to 
both `if` and `else` branches? 


-- 
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: issues-unsubscr...@flink.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [flink] snuyanzin commented on pull request #22624: [FLINK-32132][table-planner] Cast function CODEGEN does not work as e…

2023-06-20 Thread via GitHub 


snuyanzin commented on PR #22624:
URL: https://github.com/apache/flink/pull/22624#issuecomment-1599038504

   also could you please rebase to the latest master?
   The failure was fixed in later commits, so rebase should help with passing 
tests


-- 
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: issues-unsubscr...@flink.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [flink] snuyanzin commented on pull request #22624: [FLINK-32132][table-planner] Cast function CODEGEN does not work as e…

2023-06-20 Thread via GitHub 


snuyanzin commented on PR #22624:
URL: https://github.com/apache/flink/pull/22624#issuecomment-1598826176

   hmm, it looks like you renamed class however not renamed file... could you 
also please rename file in order to have them in sync?


-- 
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: issues-unsubscr...@flink.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [flink] snuyanzin commented on pull request #22624: [FLINK-32132][table-planner] Cast function CODEGEN does not work as e…

2023-06-19 Thread via GitHub 


snuyanzin commented on PR #22624:
URL: https://github.com/apache/flink/pull/22624#issuecomment-1596941999

   hi @zhougit86 
   it looks ok from my side thanks for addressing comments
   
   the only nit pick: could you please rename `CastFunctionMiscITLegacyCase` in 
way that it matches regex 
`'[A-Z][A-Za-z\d]*Test(s|Case)?|Test[A-Z][A-Za-z\d]*|IT(.*)|(.*)IT(Case)`
   e.g. something like `CastFunctionMiscLegacyITCase`


-- 
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: issues-unsubscr...@flink.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org