Skip to content

Docs: Add StarRocks to Documentation#4774

Merged
openinx merged 2 commits intoapache:masterfrom
caneGuy:zhoukang/add-sr
May 27, 2022
Merged

Docs: Add StarRocks to Documentation#4774
openinx merged 2 commits intoapache:masterfrom
caneGuy:zhoukang/add-sr

Conversation

@caneGuy
Copy link
Contributor

@caneGuy caneGuy commented May 16, 2022

This PR adds a link to the documentation for StarRocks querying for Iceberg tables.

@github-actions github-actions bot added the docs label May 16, 2022
@caneGuy caneGuy changed the title [WIP] Docs: Add StarRocks to Documentation Docs: Add StarRocks to Documentation May 16, 2022
@kbendick
Copy link
Contributor

Thanks for opening this PR and for linking the PR in the docs repo @caneGuy! I have only recently discovered StarRocks but it is very exciting. I look forward to the chance to learn more about it and hopefully use it.

In full transparency, I can't recall if the PR is supposed to be made here or in iceberg-docs, but can point out the person who will be more sure 👍

@kbendick
Copy link
Contributor

cc @samredai for visibility on including a link to external documentation for an additional query engine. There's also a PR open in iceberg-docs already linked here 👍

@caneGuy
Copy link
Contributor Author

caneGuy commented May 18, 2022

Thanks too much @kbendick

@caneGuy
Copy link
Contributor Author

caneGuy commented May 19, 2022

@samredai could you help review this pr thanks!

Copy link
Contributor

@samredai samredai left a comment

Choose a reason for hiding this comment

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

LGTM! We should merge the image in the iceberg-docs repo before this is released (there should be a docs release soon as part of the 0.13.2 release)

Copy link
Contributor

@kbendick kbendick left a comment

Choose a reason for hiding this comment

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

One thing I don't see is the actual starrocks-logo.png. Possibly that's included elsewhere though? It's a +1 from me otherwise.

EDIT: The image is in the docs repository PR. My bad.

Copy link
Contributor

@kbendick kbendick left a comment

Choose a reason for hiding this comment

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

+1 this looks good to me.

My one outstanding question, which isn't a blocker as some similar files don't even seem to have it, is whether or not the ASF license header can be at the top of the file.

But if it passes the rat license check and it does indeed have the license, I think that's sufficient given some similar files don't have it at all.

@caneGuy
Copy link
Contributor Author

caneGuy commented May 23, 2022

Thanks @kbendick @samredai
if needed, i will add ASF header

@caneGuy
Copy link
Contributor Author

caneGuy commented May 23, 2022

cc @rdblue @openinx could you help review this thanks

@caneGuy
Copy link
Contributor Author

caneGuy commented May 27, 2022

could you help merge this ? @kbendick thanks

Copy link
Member

@openinx openinx left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks @caneGuy for the great contribution !

@openinx openinx merged commit ca5c37b into apache:master May 27, 2022
@caneGuy
Copy link
Contributor Author

caneGuy commented Jun 10, 2022

@rdblue could you help reload the web site? and how can we see this logo and link on iceberg website. thanks too much

@rdblue
Copy link
Contributor

rdblue commented Jun 10, 2022

@samredai, can you help port this to the iceberg-docs repo?

@samredai
Copy link
Contributor

@rdblue this should show up in the 0.13.2 versioned docs site release which includes copying this over with the rest of the docs directory content. The logo image has already been merged (PR #81)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants