test: Add IPPREFIX to the velox fuzzer test

[mohsaka]

Adds IPPREFIX type to the velox fuzzer test. Part 2 of the 3 part split requested here.

#10816 (comment)

Cannot be merged until until the cast functions are added otherwise fuzzer will fail.

Contains changes from
#11122
To add the type without cast functions.

Example failure from the fuzzer

michaelohsaka@Michaels-MBP fuzzer % ./velox_expression_fuzzer_test –-duration_sec 30 --steps 10000 --velox_fuzzer_enable_complex_types
E20241009 14:05:37.909153 41143425 Exceptions.h:67] Line: /Users/michaelohsaka/final/velox/velox/functions/prestosql/types/IPPrefixType.cpp:63, Function:castTo, Expression:  Cast from VARCHAR to IPPrefix not yet supported, Source: RUNTIME, ErrorCode: NOT_IMPLEMENTED
E20241009 14:05:37.910630 41143425 ExpressionVerifier.cpp:146] Common eval: Exceptions other than VeloxUserError are not allowed.
libc++abi: terminating due to uncaught exception of type facebook::velox::VeloxRuntimeError: Exception: VeloxRuntimeError
Error Source: RUNTIME
Error Code: NOT_IMPLEMENTED
Reason: Cast from VARCHAR to IPPrefix not yet supported
Retriable: False
Context: cast((k/q\|S\X[C`uJ9L.t;zf<h}m;k3ia#V94oo2b187=RGfCd%`fVo]q*25tOs)yE\0%we6E?@'VXL:Z=Pk!`IA:VARCHAR) as IPPREFIX)
Function: castTo
File: /Users/michaelohsaka/final/velox/velox/functions/prestosql/types/IPPrefixType.cpp
Line: 63
Stack trace:
# 0  
# 1  
# 2  
# 3  
# 4  
# 5  
# 6  
# 7  
# 8  
# 9  
# 10 
# 11 
# 12 
# 13 
# 14 
# 15 
# 16 
# 17 
# 18 
# 19 
# 20 
# 21 
# 22 
# 23 
# 24 
# 25 
# 26 
# 27 
# 28 
# 29 
# 30 
# 31 

[netlify]

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	8a8c220
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/67589597418d0400082a50dd

[yuandagits]

#11481 is landed, this looks good! We can merge this

[facebook-github-bot]

@yuandagits has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[yuandagits]

@mohsaka this actually won't work because the ExpressionFuzzer is used by by SparkExpressionFuzzer and Spark may have different (or not even use ipprefix).

Instead we should add these special forms in ExpressionFuzzerTest since that is where it is registering the prestoSql scalar functions.

cc @kagamiori for more on fuzzers

[mohsaka]

SparkExpressionFuzzer

Will fix it. Thanks!

[mohsaka]

@yuandagits I actually think something else changed as the velox fuzzer itself doesn't even work anymore.

michaelohsaka@Michaels-MBP velox % ./_build/release/velox/expression/fuzzer/velox_expression_fuzzer_test –-duration_sec 30 --steps 10000 --velox_fuzzer_enable_complex_types
libc++abi: terminating due to uncaught exception of type facebook::velox::VeloxUserError: Exception: VeloxUserError
Error Source: USER
Error Code: INVALID_ARGUMENT
Reason: Type doesn't exist: 'IPPREFIX'
Retriable: False
Expression: hasType(typeName)
Function: validateBaseTypeAndCollectTypeParams
File: /Users/michaelohsaka/final/velox/velox/expression/FunctionSignature.cpp
Line: 124
Stack trace:
Stack trace has been disabled. Use --velox_exception_user_stacktrace_enabled=true to enable it.

zsh: abort      ./_build/release/velox/expression/fuzzer/velox_expression_fuzzer_test  30   

Will figure it out.

[yuandagits]

@yuandagits I actually think something else changed as the velox fuzzer itself doesn't even work anymore.

michaelohsaka@Michaels-MBP velox % ./_build/release/velox/expression/fuzzer/velox_expression_fuzzer_test –-duration_sec 30 --steps 10000 --velox_fuzzer_enable_complex_types
libc++abi: terminating due to uncaught exception of type facebook::velox::VeloxUserError: Exception: VeloxUserError
Error Source: USER
Error Code: INVALID_ARGUMENT
Reason: Type doesn't exist: 'IPPREFIX'
Retriable: False
Expression: hasType(typeName)
Function: validateBaseTypeAndCollectTypeParams
File: /Users/michaelohsaka/final/velox/velox/expression/FunctionSignature.cpp
Line: 124
Stack trace:
Stack trace has been disabled. Use --velox_exception_user_stacktrace_enabled=true to enable it.

zsh: abort      ./_build/release/velox/expression/fuzzer/velox_expression_fuzzer_test  30   

Will figure it out.

It is coz of initialization order of a static map.

Try rebasing off of #11770, then it shall work!

[mohsaka]

@yuandagits I actually think something else changed as the velox fuzzer itself doesn't even work anymore.

michaelohsaka@Michaels-MBP velox % ./_build/release/velox/expression/fuzzer/velox_expression_fuzzer_test –-duration_sec 30 --steps 10000 --velox_fuzzer_enable_complex_types
libc++abi: terminating due to uncaught exception of type facebook::velox::VeloxUserError: Exception: VeloxUserError
Error Source: USER
Error Code: INVALID_ARGUMENT
Reason: Type doesn't exist: 'IPPREFIX'
Retriable: False
Expression: hasType(typeName)
Function: validateBaseTypeAndCollectTypeParams
File: /Users/michaelohsaka/final/velox/velox/expression/FunctionSignature.cpp
Line: 124
Stack trace:
Stack trace has been disabled. Use --velox_exception_user_stacktrace_enabled=true to enable it.

zsh: abort      ./_build/release/velox/expression/fuzzer/velox_expression_fuzzer_test  30   

Will figure it out.

It is coz of initialization order of a static map.

Try rebasing off of #11770, then it shall work!

Thanks! That helped out. Seems like I missed adding the type to the validation as well. Added that. Everything looks good from the runs so far.

[yuandagits]

One of the test failures look to be from #11702, which is unrelated to this diff. Once all signals are green, I can try to import again. Thanks!

[facebook-github-bot]

@yuandagits has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[yuandagits]

I tried re-importing this but it causes a circular dependency issue because we're now depending on the the velox expression lib which depends on the lib containing velox/functions/prestosql/types/*, which has dependencies that then depend on the velox expression lib :(.

Another thing is, I get we're adding the !isIPPrefixName(typeName) in the FunctionSignature check to avoid the Spark test failures. But I wonder if this is too specific because the IPPrefix is currently a PrestoSQL concept.

Do we instead want to update how SparkExpressionFuzzer or is this actually okay? cc @Yuhta @kagamiori who may have better opinion on this

[mohsaka]

I tried re-importing this but it causes a circular dependency issue because we're now depending on the the velox expression lib which depends on the lib containing velox/functions/prestosql/types/*, which has dependencies that then depend on the velox expression lib :(.

Another thing is, I get we're adding the !isIPPrefixName(typeName) in the FunctionSignature check to avoid the Spark test failures. But I wonder if this is too specific because the IPPrefix is currently a PrestoSQL concept.

Do we instead want to update how SparkExpressionFuzzer or is this actually okay? cc @Yuhta @kagamiori who may have better opinion on this

From what I understand validateBaseTypeAndCollectTypeParams is velox based and should contain everything velox has. Anything that spark doesn't have should be placed in skipFunctions?

So maybe we need to add something like

cast(varchar) -> ipprefix
cast(ipprefix) -> varchar

to that.

Will need to figure out something for the circular dependency though.

[mohsaka]

@yuandagits Actually I am unable to fix this circular dependency. Interestingly it doesn't throw any error when I compile it.

The main reason is that we need to override the cast operator. The other types Decimal/Date are under Type.h though. With their compare functions in velox/expression/CastExpr.cpp.

I guess there's a reason none of the types under prestosql/types have had their cast functions added. Ex timestamp W timezone.

Any ideas?

[yuandagits]

@yuandagits Actually I am unable to fix this circular dependency. Interestingly it doesn't throw any error when I compile it.

The main reason is that we need to override the cast operator. The other types Decimal/Date are under Type.h though. With their compare functions in velox/expression/CastExpr.cpp.

I guess there's a reason none of the types under prestosql/types have had their cast functions added. Ex timestamp W timezone.

Any ideas?

Let me think more on this and get back to you!

[mohsaka]

@yuandagits Not ideal but I just put IPPREFIX name constant at the top and compare that way. Also added the skip for the cast ipprefix functions. Lets see how that goes.

[mohsaka]

@yuandagits I had a discussion with @czentgr about the circular dependency issue. We were wondering about the error hit.

1. What was the actual error hit? Was it a undefined symbol error or something else? The reason behind this is that we looked at the cmake files and it doesn't seem like velox_expression_functions links in velox_presto_types. With the change I made, we think it should have just pulled in the header of velox/functions/prestosql/types/IPPrefixType.h into velox_expression_functions through FunctionSignature.cpp. Not through a library.

2. Are you doing a static or shared build? That may be why we are not hitting the issues that you are.

3. If there is indeed a library issue, @czentgr was proposing that we separate out the cast operator into its own library to remove the dependency.

4. Fuzzer seems to be working fine with just the string. Not ideal but its working.

[yuandagits]

@yuandagits I had a discussion with @czentgr about the circular dependency issue. We were wondering about the error hit.

1. What was the actual error hit? Was it a undefined symbol error or something else? The reason behind this is that we looked at the cmake files and it doesn't seem like velox_expression_functions links in velox_presto_types. With the change I made, we think it should have just pulled in the header of velox/functions/prestosql/types/IPPrefixType.h into velox_expression_functions through FunctionSignature.cpp. Not through a library.
2. Are you doing a static or shared build? That may be why we are not hitting the issues that you are.
3. If there is indeed a library issue, @czentgr was proposing that we separate out the cast operator into its own library to remove the dependency.
4. Fuzzer seems to be working fine with just the string. Not ideal but its working.

The error was from buck, our internal build system
Configured target cycle detected (->means "depends on"): /velox/expression:velox_expression -> /velox/core:velox_core -> /velox/expression:velox_expression_functions -> /velox/functions/prestosql/types:types -> /velox/expression:velox_expression

I think with our internal build mode for testing, it is not shared. Maybe your build is static?

Not sure about adding the custom string, I'm fundamentally not opposed to it, but it does seem a bit hacky to have a custom solution. Maybe we can Look into separating the cast operator into its own lib.

[mohsaka]

@yuandagits I had a discussion with @czentgr about the circular dependency issue. We were wondering about the error hit.

1. What was the actual error hit? Was it a undefined symbol error or something else? The reason behind this is that we looked at the cmake files and it doesn't seem like velox_expression_functions links in velox_presto_types. With the change I made, we think it should have just pulled in the header of velox/functions/prestosql/types/IPPrefixType.h into velox_expression_functions through FunctionSignature.cpp. Not through a library.
2. Are you doing a static or shared build? That may be why we are not hitting the issues that you are.
3. If there is indeed a library issue, @czentgr was proposing that we separate out the cast operator into its own library to remove the dependency.
4. Fuzzer seems to be working fine with just the string. Not ideal but its working.

The error was from buck, our internal build system Configured target cycle detected (->means "depends on"): /velox/expression:velox_expression -> /velox/core:velox_core -> /velox/expression:velox_expression_functions -> /velox/functions/prestosql/types:types -> /velox/expression:velox_expression

I think with our internal build mode for testing, it is not shared. Maybe your build is static?

Not sure about adding the custom string, I'm fundamentally not opposed to it, but it does seem a bit hacky to have a custom solution. Maybe we can Look into separating the cast operator into its own lib.

Thank you for the update! Will begin looking into and working on the separated operator to see if it will work out.

[mohsaka]

@yuandagits Lets see if this will work? I removed the header files from velox/functions/prestosql/types/ and moved them into subdirectory velox/functions/prestosql/types/headers.

Ideally this should result in a dependency from velox/functions/prestosql/types->velox/functions/prestosql/types/headers
and from /velox/expression:velox_expression_functions->velox/functions/prestosql/types/headers

I may need to place it outside of types if this does not work.

All the other custom types headers can be added later as well.

[yuandagits]

@yuandagits Lets see if this will work? I removed the header files from velox/functions/prestosql/types/ and moved them into subdirectory velox/functions/prestosql/types/headers.

Ideally this should result in a dependency from velox/functions/prestosql/types->velox/functions/prestosql/types/headers and from /velox/expression:velox_expression_functions->velox/functions/prestosql/types/headers

I may need to place it outside of types if this does not work.

All the other custom types headers can be added later as well.

Let's hold off on this PR for a bit, coz I realize I will still need to change the libs internally for the mapping of the new files you've created. In parallel, I'll also try to think of an alternative way to break this dependency or a way to skip the functions