[GitHub] [kyuubi] bowenliang123 commented on a diff in pull request #4643: [KYUUBI #4530] [AUTHZ] fixbug support MASK_SHOW_FIRST_4 和 MASK_SHOW_FIRST_4 chinese data mask

["bowenliang123 (via GitHub)" <gi...@apache.org>]

bowenliang123 commented on code in PR #4643:
URL: https://github.com/apache/kyuubi/pull/4643#discussion_r1154029838


##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/SparkRangerAdminPlugin.scala:
##########
@@ -90,7 +90,7 @@ object SparkRangerAdminPlugin extends RangerBasePlugin("spark", "sparkSql")
 
   private def regexp_replace(expr: String, hasLen: Boolean = false): String = {
     val pos = if (hasLen) ", 5" else ""
-    val upper = s"regexp_replace($expr, '[A-Z]', 'X'$pos)"
+    val upper = s"regexp_replace($expr, '[A-Z\u4e00-\u9fa5]', 'X'$pos)"

Review Comment:
   Any reference for the starting and ending unicode for Chinese chars?



##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/SparkRangerAdminPlugin.scala:
##########
@@ -90,7 +90,7 @@ object SparkRangerAdminPlugin extends RangerBasePlugin("spark", "sparkSql")
 
   private def regexp_replace(expr: String, hasLen: Boolean = false): String = {
     val pos = if (hasLen) ", 5" else ""
-    val upper = s"regexp_replace($expr, '[A-Z]', 'X'$pos)"

Review Comment:
   Chinese chars should not be part of upper chars. Use a separate replacer for it.



##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/SparkRangerAdminPlugin.scala:
##########
@@ -90,7 +90,7 @@ object SparkRangerAdminPlugin extends RangerBasePlugin("spark", "sparkSql")
 
   private def regexp_replace(expr: String, hasLen: Boolean = false): String = {

Review Comment:
   Use a separate ut cases for testing this `regexp_replace` method.



##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RangerSparkExtensionSuite.scala:
##########
@@ -388,6 +389,19 @@ abstract class RangerSparkExtensionSuite extends AnyFunSuite
           assert(sql(s"SELECT value1 FROM $db.${table}2").collect() ===
             Seq(Row(DigestUtils.md5Hex("1"))))
         })
+
+      doAs(
+        "admin",
+        sql(
+          s"INSERT INTO $db.$table SELECT 10, 1, 'hello', " +
+            s"'\u6d4b\u8bd5\u4e2d\u6587\u524d\u56db\u4e2aabcABC', " +

Review Comment:
   Add comments to explain what and why to use this unicodes in tests. Source code is in UTF-8 encoding and good to have Chinese chars.



##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RangerSparkExtensionSuite.scala:
##########
@@ -342,6 +342,7 @@ abstract class RangerSparkExtensionSuite extends AnyFunSuite
         sql(
           s"INSERT INTO $db.$table SELECT 1, 1, 'hello', 'world', " +
             s"timestamp'2018-11-17 12:34:56', 'World'"))
+

Review Comment:
   Remove the unessary blank line.



-- 
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: notifications-unsubscribe@kyuubi.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org