Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[STRATCONN-3772] | Add new operator "number_equals" for number #2030

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

itsarijitray
Copy link

@itsarijitray itsarijitray commented May 13, 2024

Summary

Jira Ticket: https://segment.atlassian.net/browse/STRATCONN-3772

Addition of a new operator ("number_equals") in validate, parse-fql and generate-fql functions in @segment/destination-subscriptions.

This operator does a strict numeric equality check. This operator is only at the AST level and doesn't get stored in the actual fql. Before generating fql, This operator will be transformed into "=" without stringifying its corresponding value token.

Analysis Doc: https://docs.google.com/document/d/1hhVGplvnzTveGDpV5OS6v4tdYPSCSQiI7rLuitIyKp0/edit#heading=h.wc5kl9crigkj

!!!Important: Please follow these steps after merging.

  1. Release packages (actions-core, destination-subscription)
  2. Add the new package version of actions-core to integrations service
  3. Add the new package version of destination-subscription to this App PR.

Testing

  • Added unit tests for new functionality
  • Tested end-to-end using the local server
  • [Segmenters] Tested in the staging environment

Stage Test Doc: https://docs.google.com/document/d/1QjirLSefl3HlCphxlTgwIYn7EjO0vgMkpEOGxXMhask/edit?usp=sharing

@itsarijitray itsarijitray requested a review from a team May 13, 2024 06:54
@itsarijitray itsarijitray requested a review from a team as a code owner May 13, 2024 06:54
@Innovative-GauravKochar Innovative-GauravKochar changed the title Bug/stratconn 3772 [STRATCONN-3772] | Add new Operator "is equals to" for number May 20, 2024
@Innovative-GauravKochar Innovative-GauravKochar changed the title [STRATCONN-3772] | Add new Operator "is equals to" for number [STRATCONN-3772] | Add new operator "is equals to" for number May 20, 2024
@Innovative-GauravKochar
Copy link
Contributor

Can you please add staging testing doc here, if you have tested it in staging ?

expect(validate(ast, { properties: { value: '123' } })).toEqual(true)
expect(validate(ast, { properties: { value: 0 } })).toEqual(false)
}
})

test('operators - is equals to (==) (numbers)', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind clarifying why we are testing against string and ints on the file above but not here? Also, please describe a use case where this will be used so I can understand it a bit better. Thanks!

Copy link
Author

Choose a reason for hiding this comment

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

@marinhero Here is an analysis doc of the problem we are trying to solve. Let me know if you have any doubts.

Copy link
Contributor

@varadarajan-tw varadarajan-tw left a comment

Choose a reason for hiding this comment

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

Great analysis! Changes also look good to me. Left couple of questions.

packages/destination-subscriptions/src/generate-fql.ts Outdated Show resolved Hide resolved
@@ -165,6 +165,9 @@ const parseFqlFunction = (
}
}

const parseOperator = (valueToken: Token, operatorToken: Token) =>
valueToken.type === 'number' && operatorToken.value === '=' ? '==' : (operatorToken.value as Operator)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we override operator value = in parser? Can we try to restrict possible values on the app side?

Copy link
Author

Choose a reason for hiding this comment

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

Changes in the app is side will introduce ambiguity. Also, After the expression is formed, we would have transform this every time there is a change the expression.

Plus, App relies on this to parse FQL to AST.

@itsarijitray itsarijitray changed the title [STRATCONN-3772] | Add new operator "is equals to" for number [STRATCONN-3772] | Add new operator "number_equals" for number May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants