[VL] Pretty log native fallback reason #4494

Yohahaha · 2024-01-23T09:00:20Z

What changes were proposed in this pull request?

Introduce two new macros in native to enhance validation log: LOG_VALIDATION_MSG_FROM_EXCEPTION, LOG_VALIDATION_MSG, then help developer check root cause easily.

Before:

(4) SortAggregate: Found unsupported data type in aggregation expression,MapType(StringType,IntegerType,true), BooleanType
(9) SortAggregate: Found unsupported data type in aggregation expression,MapType(StringType,IntegerType,true), BooleanType

(4) SortAggregate: native check failure:native validation failed due to: Validation failed for input type in AggregateRel function due to:Substrait type signature conversion to Velox type not supported for list., native validation failed due to: Validation failed for ProjectRel input.
(9) SortAggregate: native check failure:native validation failed due to: Validation failed for input type in AggregateRel function due to:Substrait type signature conversion to Velox type not supported for list., native validation failed due to: Validation failed for ProjectRel input.

After:

(4) SortAggregate: Found unsupported data type in aggregation expression: first#18:MapType(StringType,IntegerType,true)
(9) SortAggregate: Found unsupported data type in aggregation expression: first(map#2)()#15:MapType(StringType,IntegerType,true)

(4) SortAggregate: Native validation failed:
  Validation failed due to exception caught at file:SubstraitToVeloxPlanValidator.cc line:962 function:validateAggRelFunctionType, thrown from file:VeloxSubstraitSignature.cc line:171 function:fromSubstraitSignature, reason:Substrait type signature conversion to Velox type not supported for list.
  Validation failed at file:SubstraitToVeloxPlanValidator.cc, line:769, function:validate, reason:ProjectRel input
(9) SortAggregate: Native validation failed:
  Validation failed due to exception caught at file:SubstraitToVeloxPlanValidator.cc line:962 function:validateAggRelFunctionType, thrown from file:VeloxSubstraitSignature.cc line:171 function:fromSubstraitSignature, reason:Substrait type signature conversion to Velox type not supported for list.
  Validation failed at file:SubstraitToVeloxPlanValidator.cc, line:769, function:validate, reason:ProjectRel input

github-actions · 2024-01-23T09:00:40Z

Thanks for opening a pull request!

Could you open an issue for this pull request on Github Issues?

https://github.com/oap-project/gluten/issues

Then could you also rename commit message and pull request title in the following format?

[GLUTEN-${ISSUES_ID}][COMPONENT]feat/fix: ${detailed message}

See also:

Other pull requests

github-actions · 2024-01-23T09:00:55Z

Run Gluten Clickhouse CI

github-actions · 2024-01-23T09:01:11Z

Run Gluten Clickhouse CI

github-actions · 2024-01-23T09:38:30Z

Run Gluten Clickhouse CI

This reverts commit 1b3d0a6.

github-actions · 2024-01-24T05:52:55Z

Run Gluten Clickhouse CI

Yohahaha · 2024-01-24T08:46:48Z

could someone help review and rerun failed CI jobs? I have checked these failed CI jobs all due to network issue. cc @PHILO-HE @ulysses-you

PHILO-HE

Thanks for your work! Just two trivial comments.

cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc

PHILO-HE · 2024-01-24T09:37:29Z

cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc

@@ -29,6 +29,20 @@
 namespace gluten {

 namespace {
+#define LOG_VALIDATION_MSG_FROM_EXCEPTION(err)                                                                                       \
+  logValidateMsg(fmt::format(                                                                                                        \
+      "Validation failed due to exception caught at file:{} line:{} function:{}, thrown from file:{} line:{} function:{} reason:{}", \


It may be better to contain exception position if debug is enabled? Ditto for LOG_VALIDATION_MSG.

I prefer keeping these information to help us quickly find root fallback reason, we may not ask customer re-run with debug mode enabled, customer's resources and patience not always enough.

github-actions · 2024-01-24T10:35:26Z

Run Gluten Clickhouse CI

ulysses-you · 2024-01-24T10:41:29Z

gluten-core/src/main/scala/io/glutenproject/extension/GlutenPlan.scala

@@ -42,7 +42,7 @@ object ValidationResult {
      ok
    } else {
      val fallbackInfo = info.getFallbackInfo.asScala
-        .mkString("native check failure:", ", ", "")
+        .mkString("Native validation failed:\n  ", "\n  ", "")


nit: remove blank after '\n'

these two blanks represent next line's indent.

(4) SortAggregate: Native validation failed: Validation failed xxx Validation failed xxx (9) SortAggregate: Native validation failed: Validation failed xxx Validation failed xxx (4) SortAggregate: Native validation failed: Validation failed xxx Validation failed xxx (9) SortAggregate: Native validation failed: Validation failed xxx Validation failed xxx

PHILO-HE · 2024-01-25T01:01:33Z

Validation failed due to exception caught at file:/root/oap-gluten/cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc line:962 function:validateAggRelFunctionType, thrown from file:/root/oap-gluten/cpp/velox/substrait/VeloxSubstraitSignature.cc line:171 function:fromSubstraitSignature reason:Substrait type signature conversion to Velox type not supported for list.

Can we just leave a file name instead of full file path? We should make it brief, as well as informative. BTW, it may look better to clearly separate reason part from thrown from xxxx part, e.g. "thrown from xxx. Reason: xxx."

github-actions · 2024-01-25T03:06:05Z

Run Gluten Clickhouse CI

Yohahaha · 2024-01-25T03:08:58Z

Validation failed due to exception caught at file:/root/oap-gluten/cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc line:962 function:validateAggRelFunctionType, thrown from file:/root/oap-gluten/cpp/velox/substrait/VeloxSubstraitSignature.cc line:171 function:fromSubstraitSignature reason:Substrait type signature conversion to Velox type not supported for list.

Can we just leave a file name instead of full file path? We should make it brief, as well as informative. BTW, it may look better to clearly separate reason part from thrown from xxxx part, e.g. "thrown from xxx. Reason: xxx."

ok.

PHILO-HE

Thanks!

ulysses-you

thank you @Yohahaha

GlutenPerfBot · 2024-01-25T06:27:25Z

===== Performance report for TPCH SF2000 with Velox backend, for reference only ====

query	log/native_4494_time.csv	log/native_master_01_24_2024_7df896bf2_time.csv	difference	percentage
q1	33.10	31.79	-1.307	96.05%
q2	23.54	23.75	0.210	100.89%
q3	36.98	37.77	0.781	102.11%
q4	37.76	37.98	0.227	100.60%
q5	68.72	66.72	-2.007	97.08%
q6	5.42	6.87	1.448	126.71%
q7	81.35	82.42	1.072	101.32%
q8	84.73	83.45	-1.283	98.49%
q9	123.82	121.64	-2.180	98.24%
q10	41.68	44.56	2.881	106.91%
q11	20.11	20.08	-0.039	99.81%
q12	26.23	28.81	2.584	109.85%
q13	44.70	44.54	-0.166	99.63%
q14	21.01	15.67	-5.337	74.60%
q15	26.60	27.54	0.941	103.54%
q16	15.10	14.10	-1.006	93.34%
q17	101.04	99.75	-1.288	98.73%
q18	145.28	144.42	-0.860	99.41%
q19	13.78	14.16	0.378	102.75%
q20	26.53	26.38	-0.152	99.43%
q21	222.85	224.22	1.369	100.61%
q22	13.55	13.45	-0.092	99.32%
total	1213.87	1210.05	-3.824	99.69%

Yohahaha added 5 commits January 24, 2024 13:39

fix

194527c

fix

8c6b7ce

fix

1b3d0a6

Revert "fix"

19c4b54

This reverts commit 1b3d0a6.

fix

1566bf7

Yohahaha force-pushed the fix-native-fallback-reason branch from 153b353 to 1566bf7 Compare January 24, 2024 05:52

PHILO-HE reviewed Jan 24, 2024

View reviewed changes

fix comments

6da35f3

ulysses-you reviewed Jan 24, 2024

View reviewed changes

fix comments

1afbc36

PHILO-HE approved these changes Jan 25, 2024

View reviewed changes

ulysses-you approved these changes Jan 25, 2024

View reviewed changes

PHILO-HE merged commit 4e33e0e into apache:main Jan 25, 2024
20 checks passed

Yohahaha deleted the fix-native-fallback-reason branch January 25, 2024 06:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[VL] Pretty log native fallback reason #4494

[VL] Pretty log native fallback reason #4494

Yohahaha commented Jan 23, 2024 •

edited

Loading

github-actions bot commented Jan 23, 2024

github-actions bot commented Jan 23, 2024

github-actions bot commented Jan 23, 2024

github-actions bot commented Jan 23, 2024

github-actions bot commented Jan 24, 2024

Yohahaha commented Jan 24, 2024

PHILO-HE left a comment

PHILO-HE Jan 24, 2024

Yohahaha Jan 24, 2024 •

edited

Loading

github-actions bot commented Jan 24, 2024

ulysses-you Jan 24, 2024

Yohahaha Jan 24, 2024

PHILO-HE commented Jan 25, 2024

github-actions bot commented Jan 25, 2024

Yohahaha commented Jan 25, 2024

PHILO-HE left a comment

ulysses-you left a comment

GlutenPerfBot commented Jan 25, 2024

[VL] Pretty log native fallback reason #4494

[VL] Pretty log native fallback reason #4494

Conversation

Yohahaha commented Jan 23, 2024 • edited Loading

What changes were proposed in this pull request?

github-actions bot commented Jan 23, 2024

github-actions bot commented Jan 23, 2024

github-actions bot commented Jan 23, 2024

github-actions bot commented Jan 23, 2024

github-actions bot commented Jan 24, 2024

Yohahaha commented Jan 24, 2024

PHILO-HE left a comment

Choose a reason for hiding this comment

PHILO-HE Jan 24, 2024

Choose a reason for hiding this comment

Yohahaha Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jan 24, 2024

ulysses-you Jan 24, 2024

Choose a reason for hiding this comment

Yohahaha Jan 24, 2024

Choose a reason for hiding this comment

PHILO-HE commented Jan 25, 2024

github-actions bot commented Jan 25, 2024

Yohahaha commented Jan 25, 2024

PHILO-HE left a comment

Choose a reason for hiding this comment

ulysses-you left a comment

Choose a reason for hiding this comment

GlutenPerfBot commented Jan 25, 2024

Yohahaha commented Jan 23, 2024 •

edited

Loading

Yohahaha Jan 24, 2024 •

edited

Loading