feat(fuzzer): Add input generator for json_parse in expression fuzzer #12019

kagamiori · 2025-01-04T04:38:48Z

Summary:
Make expression fuzzer generate input vectors of valid JSON strings for the
json_parse function. To test corner cases, the JSON strings may be
randomly truncated or inserted with a space character.

Differential Revision: D67820571

facebook-github-bot · 2025-01-04T04:39:06Z

This pull request was exported from Phabricator. Differential Revision: D67820571

netlify · 2025-01-04T04:39:09Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`40f50b3`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/67a6ae8c07947a000860bb18

facebook-github-bot · 2025-01-17T19:30:38Z

This pull request was exported from Phabricator. Differential Revision: D67820571

…facebookincubator#12019) Summary: Pull Request resolved: facebookincubator#12019 Make expression fuzzer generate input vectors of valid JSON strings for the json_parse function. To test corner cases, the JSON strings may be randomly truncated or inserted with a space character. Differential Revision: D67820571

facebook-github-bot · 2025-01-17T19:37:55Z

This pull request was exported from Phabricator. Differential Revision: D67820571

…facebookincubator#12019) Summary: Pull Request resolved: facebookincubator#12019 Make expression fuzzer generate input vectors of valid JSON strings for the json_parse function. To test corner cases, the JSON strings may be randomly truncated or inserted with a space character. Differential Revision: D67820571

facebook-github-bot · 2025-01-17T19:51:34Z

This pull request was exported from Phabricator. Differential Revision: D67820571

…facebookincubator#12019) Summary: Pull Request resolved: facebookincubator#12019 Make expression fuzzer generate input vectors of valid JSON strings for the json_parse function. To test corner cases, the JSON strings may be randomly truncated or inserted with a space character. Differential Revision: D67820571

assignUser · 2025-01-21T03:22:50Z

I assume this is also based/waiting on #12080?

…facebookincubator#12019) Summary: Make expression fuzzer generate input vectors of valid JSON strings for the json_parse function. To test corner cases, the JSON strings may be randomly truncated or inserted with a space character. Differential Revision: D67820571

facebook-github-bot · 2025-01-23T01:11:30Z

This pull request was exported from Phabricator. Differential Revision: D67820571

…facebookincubator#12019) Summary: Make expression fuzzer generate input vectors of valid JSON strings for the json_parse function. To test corner cases, the JSON strings may be randomly truncated or inserted with a space character. Differential Revision: D67820571

facebook-github-bot · 2025-01-23T01:28:59Z

This pull request was exported from Phabricator. Differential Revision: D67820571

…facebookincubator#12019) Summary: Make expression fuzzer generate input vectors of valid JSON strings for the json_parse function. To test corner cases, the JSON strings may be randomly truncated or inserted with a space character. Differential Revision: D67820571

facebook-github-bot · 2025-01-23T01:52:50Z

This pull request was exported from Phabricator. Differential Revision: D67820571

…facebookincubator#12019) Summary: Make expression fuzzer generate input vectors of valid JSON strings for the json_parse function. To test corner cases, the JSON strings may be randomly truncated or inserted with a space character. Differential Revision: D67820571

facebook-github-bot · 2025-01-27T22:38:41Z

This pull request was exported from Phabricator. Differential Revision: D67820571

kevinwilfong · 2025-02-05T17:31:24Z

velox/expression/fuzzer/ExpressionFuzzer.cpp

+  funcArgOverrides_["switch"] = std::bind(
+      &ExpressionFuzzer::generateSwitchArgs, this, std::placeholders::_1);


Is it possible to fit switch into the ArgsOverrideFunc interface? It'd be nice to get rid of this one off exceptional case (doesn't have to be done as part of this change).

Unfortunately, I don't think so. Because the function signature of "switch" only contains (conditionType, thenType, elseType). It is the generateSwitchArgs() method that randomly generate 1--5 else and then branches by calling ExpressionFuzzer::generateArg(). So generateSwitchArgs() has to be a member method of ExpressionFuzzer to be able to call generateArg().

Since the additional then and else branches are not specified in the function signature, we also cannot let generateSwitchArgs() return nullptr for them and wait for the caller of generateSwitchArgs() call generateArg().

kevinwilfong · 2025-02-05T17:34:41Z

velox/expression/fuzzer/ExpressionFuzzer.cpp

+  funcArgOverrides_[name] = std::bind(
+      func,
+      std::placeholders::_1,
+      std::cref(this->vectorFuzzer_->getOptions()),
+      std::ref(this->rng_),
+      std::ref(this->state_));
 }


Rather than using function pointers, could we make a virtual base class with a function that can be overridden?

With function pointers you just have to know the signature when you write a new one, and it will make evolving it in the future (if we need to) difficult. If we use an interface, the compiler can enforce that the signature is correct.

kevinwilfong · 2025-02-05T17:38:23Z

velox/expression/fuzzer/ExpressionFuzzer.cpp

+      VELOX_CHECK_GT(callable.args.size(), i);
+      args[i] = generateArg(callable.args[i], callable.constantArgs[i]);


nit: just use callable.args.at(i) and callable.constantArgs.at(i), no need to do the bounds check yourself

kevinwilfong · 2025-02-05T17:40:24Z

velox/expression/fuzzer/ExpressionFuzzer.h

+    int32_t remainingLevelOfNesting_;
+  };
+
+  State& stateMultable() {


stateMultable -> stateMutable

But also, why do we need to expose it?

This was for the last version of the code. I forgot to remove it. Will remove.

kevinwilfong · 2025-02-05T17:52:14Z

velox/expression/fuzzer/ExpressionFuzzer.cpp

    const std::unordered_map<std::string, std::shared_ptr<ArgGenerator>>&
-        argGenerators)
+        argGenerators,
+    const std::unordered_map<std::string, ArgsOverrideFuncPtr>&
+        argsOverrideFuncs)


From the names it's unclear what the difference is between ArgGenerator and ArgsOverrideFuncPtr, I thought ArgGenerator was doing the same thing until I read the code.

Could we rename these to something like ArgTypesGenerator and ArgValuesGenerator (or something better).

Agree. I'm renaming them in #12283.

…facebookincubator#12019) Summary: Make expression fuzzer generate input vectors of valid JSON strings for the json_parse function. To test corner cases, the JSON strings may be randomly truncated or inserted with a space character. Differential Revision: D67820571

facebook-github-bot · 2025-02-07T05:07:42Z

This pull request was exported from Phabricator. Differential Revision: D67820571

…facebookincubator#12019) Summary: Make expression fuzzer generate input vectors of valid JSON strings for the json_parse function. To test corner cases, the JSON strings may be randomly truncated or inserted with a space character. Differential Revision: D67820571

facebook-github-bot · 2025-02-07T18:13:00Z

This pull request was exported from Phabricator. Differential Revision: D67820571

…facebookincubator#12019) Summary: Make expression fuzzer generate input vectors of valid JSON strings for the json_parse function. To test corner cases, the JSON strings may be randomly truncated or inserted with a space character. Differential Revision: D67820571

facebook-github-bot · 2025-02-07T18:15:49Z

This pull request was exported from Phabricator. Differential Revision: D67820571

kevinwilfong

Thanks for the updates!

kevinwilfong · 2025-02-07T19:31:12Z

velox/expression/fuzzer/ArgsOverrideFunctions.h

+
+namespace facebook::velox::fuzzer {
+
+class generateJsonParseArgs : public ArgValuesGenerator {


class names should be upper case: GenerateJsonParseArgs

nit: JsonParseArgsGenerator?

kevinwilfong · 2025-02-07T19:32:20Z

velox/expression/fuzzer/ArgsOverrideFunctions.cpp

+/*std::vector<core::TypedExprPtr> generateJsonParseArg(
+    const CallableSignature& signature,
+    const VectorFuzzer::Options& options,
+    FuzzerGenerator& rng,
+    ExpressionFuzzer::State& state) {
+  VELOX_CHECK_EQ(signature.args.size(), 1);
+  std::vector<core::TypedExprPtr> inputExpressions;
+
+  state.inputRowTypes_.emplace_back(signature.args[0]);
+  state.inputRowNames_.emplace_back(
+      fmt::format("c{}", state.inputRowTypes_.size() - 1));
+
+  const auto representedType = facebook::velox::randType(rng, 3);
+  const auto seed = rand<uint32_t>(rng);
+  const auto nullRatio = options.nullRatio;
+  state.customInputGenerators_.emplace_back(
+      std::make_shared<fuzzer::JsonInputGenerator>(
+          seed,
+          signature.args[0],
+          nullRatio,
+          fuzzer::getRandomInputGenerator(seed, representedType, nullRatio),
+          true));
+
+  inputExpressions.push_back(std::make_shared<core::FieldAccessTypedExpr>(
+      signature.args[0], state.inputRowNames_.back()));
+  return inputExpressions;
+}*/


This can be deleted

Forgot it. Thank you for catching this.

kevinwilfong · 2025-02-07T19:34:34Z

velox/expression/fuzzer/ExpressionFuzzer.cpp

+  // Special case for switch because it has a variable number of arguments not
+  // specified in the signature. Other functions' argument override should be
+  // specified through funcArgOverrides_.
+  if (callable.name == "switch") {
+    return generateSwitchArgs(callable);
+  }


Nice! I like this better!

kevinwilfong · 2025-02-07T19:36:00Z

velox/expression/fuzzer/ExpressionFuzzer.h

+  /// nullptr at the corresponding index. ExpressionFuzzer then generates random
+  /// arguments for these unspecified ones with the types specified in the
+  /// function signature. (Functions of variable arity must determine the number
+  /// of arguments in the overridden method. Arguments at indices beyong the


nit: beyong -> beyond

…facebookincubator#12019) Summary: Make expression fuzzer generate input vectors of valid JSON strings for the json_parse function. To test corner cases, the JSON strings may be randomly truncated or inserted with a space character. Reviewed By: kevinwilfong Differential Revision: D67820571

facebook-github-bot · 2025-02-08T01:08:41Z

This pull request was exported from Phabricator. Differential Revision: D67820571

facebook-github-bot · 2025-02-08T01:08:45Z

This pull request was exported from Phabricator. Differential Revision: D67820571

kagamiori requested review from assignUser and majetideepak as code owners January 4, 2025 04:38

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 4, 2025

facebook-github-bot added the fb-exported label Jan 4, 2025

kagamiori force-pushed the export-D67820571 branch from 3c0d260 to d72e075 Compare January 17, 2025 19:30

kagamiori force-pushed the export-D67820571 branch from d72e075 to 6f75498 Compare January 17, 2025 19:38

kagamiori requested a review from kevinwilfong January 17, 2025 19:38

kagamiori force-pushed the export-D67820571 branch from 6f75498 to 38a3cf0 Compare January 17, 2025 19:51

kagamiori force-pushed the export-D67820571 branch from 38a3cf0 to 822f9c3 Compare January 23, 2025 01:10

kagamiori force-pushed the export-D67820571 branch from 822f9c3 to 2f60556 Compare January 23, 2025 01:28

kagamiori force-pushed the export-D67820571 branch from 2f60556 to 6ca61e0 Compare January 23, 2025 01:52

kagamiori force-pushed the export-D67820571 branch from 6ca61e0 to 9fec9e9 Compare January 27, 2025 22:38

kevinwilfong reviewed Feb 5, 2025

View reviewed changes

kagamiori force-pushed the export-D67820571 branch from 9fec9e9 to e0b4cd4 Compare February 7, 2025 05:07

kagamiori force-pushed the export-D67820571 branch from e0b4cd4 to 9cf9fb0 Compare February 7, 2025 18:12

kagamiori force-pushed the export-D67820571 branch from 9cf9fb0 to c103528 Compare February 7, 2025 18:15

kagamiori requested a review from kevinwilfong February 7, 2025 19:15

kevinwilfong approved these changes Feb 7, 2025

View reviewed changes

kagamiori force-pushed the export-D67820571 branch 2 times, most recently from 4bd683e to 40f50b3 Compare February 8, 2025 01:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(fuzzer): Add input generator for json_parse in expression fuzzer #12019

feat(fuzzer): Add input generator for json_parse in expression fuzzer #12019

kagamiori commented Jan 4, 2025

facebook-github-bot commented Jan 4, 2025

netlify bot commented Jan 4, 2025 •

edited

Loading

facebook-github-bot commented Jan 17, 2025

facebook-github-bot commented Jan 17, 2025

facebook-github-bot commented Jan 17, 2025

assignUser commented Jan 21, 2025

facebook-github-bot commented Jan 23, 2025

facebook-github-bot commented Jan 23, 2025

facebook-github-bot commented Jan 23, 2025

facebook-github-bot commented Jan 27, 2025

kevinwilfong Feb 5, 2025

kagamiori Feb 6, 2025

kevinwilfong Feb 5, 2025

kevinwilfong Feb 5, 2025

kevinwilfong Feb 5, 2025

kevinwilfong Feb 5, 2025

kagamiori Feb 6, 2025

kevinwilfong Feb 5, 2025

kagamiori Feb 7, 2025

facebook-github-bot commented Feb 7, 2025

facebook-github-bot commented Feb 7, 2025

facebook-github-bot commented Feb 7, 2025

kevinwilfong left a comment

kevinwilfong Feb 7, 2025

kevinwilfong Feb 7, 2025

kagamiori Feb 7, 2025

kevinwilfong Feb 7, 2025

kevinwilfong Feb 7, 2025

facebook-github-bot commented Feb 8, 2025

facebook-github-bot commented Feb 8, 2025

		funcArgOverrides_["switch"] = std::bind(
		&ExpressionFuzzer::generateSwitchArgs, this, std::placeholders::_1);

		VELOX_CHECK_GT(callable.args.size(), i);
		args[i] = generateArg(callable.args[i], callable.constantArgs[i]);


		namespace facebook::velox::fuzzer {

		class generateJsonParseArgs : public ArgValuesGenerator {

feat(fuzzer): Add input generator for json_parse in expression fuzzer #12019

Are you sure you want to change the base?

feat(fuzzer): Add input generator for json_parse in expression fuzzer #12019

Conversation

kagamiori commented Jan 4, 2025

facebook-github-bot commented Jan 4, 2025

netlify bot commented Jan 4, 2025 • edited Loading

✅ Deploy Preview for meta-velox canceled.

facebook-github-bot commented Jan 17, 2025

facebook-github-bot commented Jan 17, 2025

facebook-github-bot commented Jan 17, 2025

assignUser commented Jan 21, 2025

facebook-github-bot commented Jan 23, 2025

facebook-github-bot commented Jan 23, 2025

facebook-github-bot commented Jan 23, 2025

facebook-github-bot commented Jan 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Feb 7, 2025

facebook-github-bot commented Feb 7, 2025

facebook-github-bot commented Feb 7, 2025

kevinwilfong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Feb 8, 2025

facebook-github-bot commented Feb 8, 2025

netlify bot commented Jan 4, 2025 •

edited

Loading