advancedxy commented on code in PR #10031:
URL: 
https://github.com/apache/arrow-datafusion/pull/10031#discussion_r1564132402


##########
datafusion/physical-expr/src/expressions/in_list.rs:
##########
@@ -1314,4 +1340,96 @@ mod tests {
 
         Ok(())
     }
+
+    #[test]
+    fn in_list_utf8_with_dict_types() -> Result<()> {
+        fn dict_lit(key_type: DataType, value: &str) -> Arc<dyn PhysicalExpr> {
+            lit(ScalarValue::Dictionary(
+                Box::new(key_type),
+                Box::new(ScalarValue::new_utf8(value.to_string())),
+            ))
+        }
+
+        fn null_dict_lit(key_type: DataType) -> Arc<dyn PhysicalExpr> {
+            lit(ScalarValue::Dictionary(
+                Box::new(key_type),
+                Box::new(ScalarValue::Utf8(None)),
+            ))
+        }
+
+        let schema = Schema::new(vec![Field::new(
+            "a",
+            DataType::Dictionary(Box::new(DataType::UInt16), 
Box::new(DataType::Utf8)),
+            true,
+        )]);
+        let a: UInt16DictionaryArray =
+            vec![Some("a"), Some("d"), None].into_iter().collect();
+        let col_a = col("a", &schema)?;
+        let batch = RecordBatch::try_new(Arc::new(schema.clone()), 
vec![Arc::new(a)])?;
+
+        // expression: "a in ("a", "b")"
+        let lists = [
+            vec![lit("a"), lit("b")],
+            vec![
+                dict_lit(DataType::Int8, "a"),
+                dict_lit(DataType::UInt16, "b"),
+            ],
+        ];
+        for list in lists.iter() {
+            in_list_raw!(

Review Comment:
   > Got it -- what I don't understand is how these validate the Comet use 
case. I expect them to call in_list (instead they calling the in_list_raw! 
macro)
   
   There might be some misunderstanding here. `in_list` and `in_list_raw` are 
both test **macros**, the main difference is that the former performs type 
coercion first and the latter does not. These two macros both call the 
`in_list` **method** though. The comet case calls the `in_list` **method**(not 
the test macro) directly without type coercion, which is exactly the 
`in_list_utf8_with_dict_types` test trying to simulate by calling the 
`in_list_raw` test **macro**.
   
   > Sorry to be so pedantic about this, but I think it is somewhat subtle so 
making sure we get it right (and don't accidentally break it in the future) I 
think is important
   
   No worries. I think it's exactly the spirit we need towards a better 
software engineering practice. And totally agree that it's important to make 
sure we get it right and won't break it in the future.



-- 
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]

Reply via email to