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

Nitpickings #13

Merged
merged 17 commits into from
Nov 19, 2020
Merged

Nitpickings #13

merged 17 commits into from
Nov 19, 2020

Conversation

szarnyasg
Copy link
Collaborator

@szarnyasg szarnyasg commented Nov 15, 2020

I started going through the docs and iron out some inconsistencies. Most of them are fairly straightforward so instead of opening a dozen PRs, I've batched them in a single one. The commit messages describe the changes in detail.

I have a few questions:

  1. Is there a reason <em> texts are rendered as normal (and not italic)?

  2. How should we spell type names? Currently it's a mix of all lowercase and all uppercase:

    $ ag bigint -s --no-group | wc -l
    6
    
    $ ag BIGINT -s --no-group | wc -l
    4
    

    I'm fine both ways but it should be consistent.

  3. Is CHAR an alias for VARCHAR?

  4. In this sentence, there seems to be a word missing: "DuckDB contains a columnar-vectorized query execution engine, where queries are still interpreted, but a large batch of values from a single (a “vector”) are processed in one operation" -> from a single what? "chunk"?

@Mytherin
Copy link
Collaborator

Is there a reason <em> texts are rendered as normal (and not italic)?

I don't know, that seems like a mistake (but you would need to ask the designers there).

How should we spell type names? Currently it's a mix of all lowercase and all uppercase:

My vote goes to all uppercase for clarity.

Is CHAR an alias for VARCHAR?

Yes, so is STRING, etc. We currently don't support a "proper" CHAR type that includes padding with spaces on output, and I'm not sure that we ever will. It seems like legacy to me that was introduced for database developer convenience. I can't really envision a use case for CHAR instead of VARCHAR.

In this sentence, there seems to be a word missing: "DuckDB contains a columnar-vectorized query execution engine, where queries are still interpreted, but a large batch of values from a single (a “vector”) are processed in one operation" -> from a single what? "chunk"?

I think the from a single need to go, so the sentence reads like: "DuckDB contains a columnar-vectorized query execution engine, where queries are still interpreted, but a large batch of values (a “vector”) are processed in one operation"

@szarnyasg
Copy link
Collaborator Author

Thanks.

Regarding <em>, there is another inconsistency: if it is inside a list, it's italic+monospace:

image

How do I communicate with the designers?

@szarnyasg
Copy link
Collaborator Author

szarnyasg commented Nov 16, 2020

The minvalue (descending) / maxvalue (ascending) defaults in https://duckdb.org/docs/sql/statements/create_sequence seem incorrect.

The code states:

				if (info->increment < 0) {
					info->start_value = info->max_value = -1;
					info->min_value = NumericLimits<int64_t>::Minimum();
				} else {
					info->start_value = info->min_value = 1;
					info->max_value = NumericLimits<int64_t>::Maximum();
				}
int64_t NumericLimits<int64_t>::Minimum() {
	return numeric_limits<int64_t>::lowest() + 1;
}

int64_t NumericLimits<int64_t>::Maximum() {
	return numeric_limits<int64_t>::max();
}

If I run

  std::cout << "Minimum value: " << (std::numeric_limits<int64_t>::lowest() + 1) << '\n';
  std::cout << "Maximum value: " << (std::numeric_limits<int64_t>::max()) << '\n';
}

I get

Minimum value: -9223372036854775807
Maximum value: 9223372036854775807

So they seem plus-minus (2^63 - 1).

@Mytherin
Copy link
Collaborator

This is related to null values within the domain that are still in some places in the system. Once those are removed the minimum value will be updated to those bounds. I think this is not worth changing since it will need to be updated later on again when we solve that issue (which we will likely forget :)).

@szarnyasg szarnyasg marked this pull request as ready for review November 16, 2020 11:14
@Mytherin
Copy link
Collaborator

Thanks for the changes. Can you resolve the merge conflict when you have time? After that I think it is ready to be merged.

@Mytherin
Copy link
Collaborator

Thanks!

@Mytherin Mytherin merged commit d00f796 into duckdb:master Nov 19, 2020
@szarnyasg szarnyasg deleted the nitpickings branch December 3, 2021 05:14
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

2 participants