Skip to content

Minor: fix wrong function call#8847

Merged
alamb merged 2 commits into
apache:mainfrom
Weijun-H:chore
Jan 15, 2024
Merged

Minor: fix wrong function call#8847
alamb merged 2 commits into
apache:mainfrom
Weijun-H:chore

Conversation

@Weijun-H

@Weijun-H Weijun-H commented Jan 13, 2024

Copy link
Copy Markdown
Member

Which issue does this PR close?

Closes #.

Rationale for this change

This issue is called #8744 , but I am curious why it passed the ci 🤔

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@alamb alamb left a comment

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.

This seems like it is an oversight in our CI -- is it possible to write a test that would fail without this code change 🤔

Nice catch

@Weijun-H

Copy link
Copy Markdown
Member Author

This seems like it is an oversight in our CI -- is it possible to write a test that would fail without this code change 🤔

Nice catch

The problem may not related to ci, because I tested it on cli, and it also worked. @alamb

❯ select array_resize([1,2],5);
+------------------------------------------------------+
| array_resize(make_array(Int64(1),Int64(2)),Int64(5)) |
+------------------------------------------------------+
| [1, 2, , , ]                                         |
+------------------------------------------------------+
1 row in set. Query took 0.009 seconds.

@comphead comphead left a comment

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.

lgtm thanks @Weijun-H

@alamb

alamb commented Jan 14, 2024

Copy link
Copy Markdown
Contributor

The problem may not related to ci, because I tested it on cli, and it also worked. @alamb

Right, sorry, what I was saying is that given there is a bug you found in the code (by manual inspection) but

  1. it wasn't found by tests
  2. you didn't have to update any tests when you changed the code

I conclude there is a gap in our test coverage. Thus I was suggesting we find a way to write a test that would fail prior to this code change, but will pass with the change

@Weijun-H

Copy link
Copy Markdown
Member Author

The additional tests is in #8868 @alamb

@alamb alamb merged commit 6b8c6ad into apache:main Jan 15, 2024
@alamb

alamb commented Jan 15, 2024

Copy link
Copy Markdown
Contributor

Thanks again @Weijun-H

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.

3 participants