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

BF-33222 set binSize to null instead of "" (empty string) #1220

Merged
merged 1 commit into from May 16, 2024

Conversation

lgfang
Copy link
Collaborator

@lgfang lgfang commented May 13, 2024

Jira Ticket: BF-33222

Whats Changed:

Set a "binSize" value to null. Originally it lacked a value. During YAML linting and formatting, we set its value to an empty string. Now, change it to null as the corresponding test failed with the value being an empty string.

Patch testing results:

patch

Related PRs:

If applicable, link related PRs

@lgfang lgfang marked this pull request as ready for review May 13, 2024 01:36
@lgfang lgfang requested a review from a team as a code owner May 13, 2024 01:36
@lgfang lgfang requested a review from jawwadasghar May 13, 2024 01:36
@thessem thessem enabled auto-merge May 16, 2024 00:43
@thessem thessem requested a review from jimoleary May 16, 2024 00:43
Copy link
Contributor

@jimoleary jimoleary left a comment

Choose a reason for hiding this comment

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

LGTM

Ugh looks like fixing the following went from an implicit null to an explicit "":

      pipeline: [{$sort: {"time": 1}},
                 {$addFields: {
                    "t": {$dateTrunc: {
                    date: "$time",
                    unit: "second",
                    binSize
                  }}

We might want to look through the changes to see if there are any more but I'm not sure how we would do that automatically :-(

@thessem thessem added this pull request to the merge queue May 16, 2024
Merged via the queue into master with commit 65e1858 May 16, 2024
11 checks passed
@thessem
Copy link
Collaborator

thessem commented May 20, 2024

@jimoleary if it makes you feel less anxious I did pick up the inconsistency in #1205 (review), it just seems like the fix was the wrong way of going about it. It was only the binSize and some of the octal->non-octal changes that worry me, so this PR removes half my worries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants