Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion core/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,33 @@ type actionsWithDependencies =
export const nativeRequire =
typeof __webpack_require__ === "function" ? __non_webpack_require__ : require;

// Turn a selector pattern containing '*' wildcards into an anchored RegExp.
// Every character except '*' is matched literally (regex metacharacters are
// escaped); each '*' matches any run of characters, so "mrd*" -> /^mrd.*$/ and
// "*features*" -> /^.*features.*$/.
function globToRegExp(pattern: string): RegExp {
const escaped = pattern.replace(/[.*+?^${}()|[\]\\]/g, "\\$&").replace(/\\\*/g, ".*");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you simplify this logic?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, that was doing too much at once — escaping every metacharacter including *, then
un-escaping \* back into .*. You had to read both replaces together to see what it did.

Rewritten to split on the wildcards, escape the literal parts, and rejoin:

function globToRegExp(pattern: string): RegExp {
  const escapeLiteral = (literal: string) => literal.replace(/[.+?^${}()|[\]\\]/g, "\\$&");
  return new RegExp(`^${pattern.split("*").map(escapeLiteral).join(".*")}$`);
}

Behaviour is unchanged — the existing matchPatterns tests pass, and I diffed the two
implementations exhaustively over patterns covering every regex metacharacter plus ** and a**b.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(note that this change isn't pushed yet, awaiting response)

return new RegExp(`^${escaped}$`);
}

export function matchPatterns(patterns: string[], values: string[]) {
const fullyQualifiedActions: string[] = [];
patterns.forEach(pattern => {
if (pattern.includes(".")) {
if (pattern.includes("*")) {
// Wildcard selector. A pattern that contains "." matches against the
// fully-qualified action name; otherwise it matches against the unqualified
// name (last segment), mirroring the exact-match branches below. Wildcards
// are expected to select many actions, so no ambiguity error applies here.
const regExp = globToRegExp(pattern);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't you first to split by components and then apply regexes?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to switch if that's the semantics you want, but it isn't behaviour-neutral, so I'd rather
confirm the intent than guess.

I implemented component-wise matching (split pattern and value on ., require the same number of
parts, apply one regex per part) and diffed it against the current implementation. One case
changes: whether * may cross a dot.

The values here are targetAsReadableString output — project.dataset.name, or dataset.name
when defaultProject isn't set. On the usual three-part name:

pattern current component-wise
*, orders, orders* same same
*.dataset.orders, *.dataset.* same same
*.orders every …orders action nothing

Component-wise is the conventional glob rule (* stops at the separator, like shell * and /),
and I'm not against it. The one thing that gives me pause is that --actions "*.orders" reads like
"the orders table in whichever dataset", and component-wise it selects nothing on a three-part
name — the user has to know to write *.*.orders. Since wildcards deliberately don't raise the
no-match/ambiguity error, that failure is silent. Letting * span dots avoids it, at the cost of
being less strict.

Which would you prefer? If component-wise, I'll push it with tests pinning the part-count
behaviour explicitly.

(Probably out of scope here, but worth separating out: dataset.* matches nothing under either
scheme on a three-part name, because a pattern containing . is matched against the fully-qualified
name. Exact dataset.orders behaves the same way today, so the wildcard branch is at least
consistent with the existing rule — happy to look at that separately if it's worth changing.)

const scope = pattern.includes(".")
? values
: values.map(value => value.split(".").slice(-1)[0]);
values.forEach((value, i) => {
if (regExp.test(scope[i])) {
fullyQualifiedActions.push(value);
}
});
} else if (pattern.includes(".")) {
if (values.includes(pattern)) {
fullyQualifiedActions.push(pattern);
}
Expand Down
64 changes: 64 additions & 0 deletions core/utils_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
getEffectiveTableFolderSubpath,
getFileFormatValueForIcebergTable,
getStorageUriForIcebergTable,
matchPatterns,
validateConnectionFormat,
validateNoMixedCompilationMode,
validateStorageUriFormat,
Expand Down Expand Up @@ -299,4 +300,67 @@ suite('Dataform Utility Validations', () => {
);
});
});

suite('matchPatterns', () => {
const values = [
'schema.mrd_features_inference',
'schema.mrd_features_training',
'other.customer_orders',
'analytics.mrd_summary',
];

test('exact unqualified name selects the single matching action', () => {
expect(matchPatterns(['mrd_features_inference'], values)).to.deep.equal([
'schema.mrd_features_inference',
]);
});

test('exact fully-qualified name selects that action', () => {
expect(matchPatterns(['other.customer_orders'], values)).to.deep.equal([
'other.customer_orders',
]);
});

test('ambiguous unqualified exact name still throws', () => {
// Two schemas, same unqualified name.
const dupes = ['a.dup', 'b.dup'];
expect(() => matchPatterns(['dup'], dupes)).to.throw();
});

test('bare "*" matches every action', () => {
expect(matchPatterns(['*'], values)).to.deep.equal(values);
});

test('prefix wildcard matches on the unqualified name', () => {
expect(matchPatterns(['mrd*'], values)).to.deep.equal([
'schema.mrd_features_inference',
'schema.mrd_features_training',
'analytics.mrd_summary',
]);
});

test('surrounding wildcards match a substring of the unqualified name', () => {
expect(matchPatterns(['*features*'], values)).to.deep.equal([
'schema.mrd_features_inference',
'schema.mrd_features_training',
]);
});

test('qualified wildcard matches against the fully-qualified name', () => {
expect(matchPatterns(['schema.*'], values)).to.deep.equal([
'schema.mrd_features_inference',
'schema.mrd_features_training',
]);
});

test('wildcard with no matches returns empty (no ambiguity error)', () => {
expect(matchPatterns(['nope*'], values)).to.deep.equal([]);
});

test('literal dot in a qualified wildcard is not a regex wildcard', () => {
// "schemaXmrd..." must NOT match "schema.*" — the "." is literal.
const tricky = ['schema.mrd_a', 'schemaXmrd_b'];
expect(matchPatterns(['schema.*'], tricky)).to.deep.equal(['schema.mrd_a']);
});
});
});