Skip to content

Commit

Permalink
Replace the overuse of StringView with std::string_view in Re2Function (
Browse files Browse the repository at this point in the history
facebookincubator#8047)

Summary:
Currently there are some overuse of StringView in Re2Functions.h/cpp. It's not good because StringView is not a replacement of std::string_view, its copy is expensive than std::string_view, its data() is not safe to point to for copied short StringView.

In this PR we replace StringView with std::string_view if it is a field of struct or a function param which is not feed by XxxVector directly.

Pull Request resolved: facebookincubator#8047

Reviewed By: Yuhta

Differential Revision: D52167089

Pulled By: mbasmanova

fbshipit-source-id: 63e2fdf85291a7ab67c9cde39f0ae7c5445a2d10
  • Loading branch information
xumingming authored and facebook-github-bot committed Dec 15, 2023
1 parent 1b98f0a commit 2106478
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 16 deletions.
19 changes: 9 additions & 10 deletions velox/functions/lib/Re2Functions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,6 @@
#include <optional>
#include <string>

#include "velox/expression/VectorWriters.h"
#include "velox/type/StringView.h"
#include "velox/vector/BaseVector.h"

namespace facebook::velox::functions {
namespace {

Expand Down Expand Up @@ -594,7 +590,7 @@ class LikeGeneric final : public VectorFunction {
const StringView& pattern,
const std::optional<char>& escapeChar) -> bool {
PatternMetadata patternMetadata =
determinePatternKind(pattern, escapeChar);
determinePatternKind(std::string_view(pattern), escapeChar);
const auto reducedLength = patternMetadata.length;
const auto& fixedPattern = patternMetadata.fixedPattern;

Expand Down Expand Up @@ -971,7 +967,7 @@ std::vector<std::shared_ptr<exec::FunctionSignature>> re2ExtractSignatures() {
}

std::string unescape(
StringView pattern,
std::string_view pattern,
size_t start,
size_t end,
std::optional<char> escapeChar) {
Expand Down Expand Up @@ -1023,7 +1019,9 @@ std::string unescape(
// Iterates through a pattern string. Transparently handles escape sequences.
class PatternStringIterator {
public:
PatternStringIterator(StringView pattern, std::optional<char> escapeChar)
PatternStringIterator(
std::string_view pattern,
std::optional<char> escapeChar)
: pattern_(pattern), escapeChar_(escapeChar) {}

// Advance the cursor to next char, escape char is automatically handled.
Expand Down Expand Up @@ -1118,7 +1116,7 @@ class PatternStringIterator {
return pattern_.data()[index];
}

const StringView pattern_;
std::string_view pattern_;
const std::optional<char> escapeChar_;

size_t currentStart_{0};
Expand All @@ -1128,7 +1126,7 @@ class PatternStringIterator {
};

PatternMetadata determinePatternKind(
StringView pattern,
std::string_view pattern,
std::optional<char> escapeChar) {
const size_t patternLength = pattern.size();

Expand Down Expand Up @@ -1279,7 +1277,8 @@ std::shared_ptr<exec::VectorFunction> makeLike(

PatternMetadata patternMetadata;
try {
patternMetadata = determinePatternKind(pattern, escapeChar);
patternMetadata =
determinePatternKind(std::string_view(pattern), escapeChar);
} catch (...) {
return std::make_shared<exec::AlwaysFailingVectorFunction>(
std::current_exception());
Expand Down
2 changes: 1 addition & 1 deletion velox/functions/lib/Re2Functions.h
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ std::vector<std::shared_ptr<exec::FunctionSignature>> re2ExtractSignatures();
/// characters} for patterns with wildcard characters only. Return
/// {kGenericPattern, 0} for generic patterns).
PatternMetadata determinePatternKind(
StringView pattern,
std::string_view pattern,
std::optional<char> escapeChar);

std::shared_ptr<exec::VectorFunction> makeLike(
Expand Down
10 changes: 5 additions & 5 deletions velox/functions/lib/tests/Re2FunctionsTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -462,16 +462,16 @@ TEST_F(Re2FunctionsTest, likePattern) {

TEST_F(Re2FunctionsTest, likeDeterminePatternKind) {
auto testPattern =
[&](StringView pattern, PatternKind patternKind, vector_size_t length) {
[&](std::string_view pattern, PatternKind patternKind, size_t length) {
PatternMetadata patternMetadata =
determinePatternKind(pattern, std::nullopt);
EXPECT_EQ(patternMetadata.patternKind, patternKind);
EXPECT_EQ(patternMetadata.length, length);
};

auto testPatternString = [&](StringView pattern,
auto testPatternString = [&](std::string_view pattern,
PatternKind patternKind,
StringView fixedPattern) {
std::string_view fixedPattern) {
PatternMetadata patternMetadata =
determinePatternKind(pattern, std::nullopt);
EXPECT_EQ(patternMetadata.patternKind, patternKind);
Expand Down Expand Up @@ -526,9 +526,9 @@ TEST_F(Re2FunctionsTest, likeDeterminePatternKind) {
}

TEST_F(Re2FunctionsTest, likeDeterminePatternKindWithEscapeChar) {
auto testPattern = [&](StringView pattern,
auto testPattern = [&](std::string_view pattern,
PatternKind patternKind,
StringView fixedPattern) {
std::string_view fixedPattern) {
PatternMetadata patternMetadata = determinePatternKind(pattern, '\\');
EXPECT_EQ(patternMetadata.patternKind, patternKind);
EXPECT_EQ(patternMetadata.length, fixedPattern.size());
Expand Down

0 comments on commit 2106478

Please sign in to comment.