Fix typos and possibly a bug?

[BenSandeen]

Fix a typo in comments, fix some whitespace, and possibly fix a bug??

I haven't been able to get the tests running, so I'm sorry if this fails tests

This is an attempt to resolve this issue: #508

[CLAassistant]

All committers have signed the CLA.

[rtwalker]

Thanks for the PR!

I haven't been able to get the tests running, so I'm sorry if this fails tests

I just kicked them off, so we'll find out shortly :)

Were you trying cargo pgx test pg14? It's good for us to know when things don't work for others.

[BenSandeen]

Were you trying cargo pgx test pg14? It's good for us to know when things don't work for others.

I was attempting to run cargo pgx test pg12; I'd have preferred to test against PG 14, but pgx's installation somehow only managed to get PG 12 up and running (despite saying it was installing PG 11, PG 12, PG 13, and PG 14). And which directory am I supposed to run the tests in? I tried both the root directory and extension.

[BenSandeen]

I definitely don't fully understand this code yet, but it seems suspicious that we're using r instead of l here. I could certainly be mistaken, though

[WireBaron]

No, this is definitely a bug. Great catch!

[rtwalker]

I was attempting to run cargo pgx test pg12; I'd have preferred to test against PG 14, but pgx's installation somehow only managed to get PG 12 up and running (despite saying it was installing PG 11, PG 12, PG 13, and PG 14). And which directory am I supposed to run the tests in? I tried both the root directory and extension.

CI is reporting tests passing and I think it's OK if you just want to rely on that for this size of PR.

If you still want to get a setup that you can use for developing and testing locally though I'm happy to try to assist. Otherwise, just ignore the below :)

To tell pgx to only care about PG14 during init, you can either

• cargo pgx init --pg14 /path/to/pg_config if you have a local PostgreSQL 14 installed that you want pgx to use
• cargo pgx init --pg14 download to use a PostgreSQL installation that pgx will download and manage for testing

Tests should be run from the extension/ subdirectory. (I don't love it, but perhaps we can at least make that explicit in the README)

[JLockerman]

Yup, that looks like a bug in the bias correction code. I suspect it's a significant contributor to issue #508

[BenSandeen]

Yup, that looks like a bug in the bias correction code. I suspect it's a significant contributor to issue #508

I spent some more time thinking about this, and I'm less confident than I originally was that it'll completely resolve that issue (although I still don't fully understand this code :P ). There are some tests that are still passing when run against this PR that assert that the estimate_count() method returns values with greater error than we'd expect. Specifically, the following tests that use a value for b of 12 or greater that assert a relative error of greater than 10%:

• timescaledb-toolkit/crates/hyperloglogplusplus/src/dense.rs

  Lines 302 to 309 in 64ddef1

  	#[test]
  	fn add_b12_n1k() {
  	let mut hll = Storage::new(12);
  	for i in 0..1000 {
  	hll.add_hash(hash(i));
  	}
  	assert_eq!(hll.estimate_count(), 1146);
  	}

• timescaledb-toolkit/crates/hyperloglogplusplus/src/dense.rs

  Lines 329 to 336 in 64ddef1

  	#[test]
  	fn add_b12_n10k() {
  	let mut hll = Storage::new(12);
  	for i in 0..10000 {
  	hll.add_hash(hash(i));
  	}
  	assert_eq!(hll.estimate_count(), 11513);
  	}

• timescaledb-toolkit/crates/hyperloglogplusplus/src/dense.rs

  Lines 347 to 354 in 64ddef1

  	#[test]
  	fn add_b16_n100k() {
  	let mut hll = Storage::new(16);
  	for i in 0..100000 {
  	hll.add_hash(hash(i));
  	}
  	assert_eq!(hll.estimate_count(), 126_448);
  	}

• timescaledb-toolkit/crates/hyperloglogplusplus/src/dense.rs

  Lines 356 to 363 in 64ddef1

  	#[test]
  	fn add_b16_n1m() {
  	let mut hll = Storage::new(16);
  	for i in 0..1000000 {
  	hll.add_hash(hash(i));
  	}
  	assert_eq!(hll.estimate_count(), 882644);
  	}

[WireBaron]

No, this is definitely a bug. Great catch!

[WireBaron]

bors r+

[bors]

Build succeeded:

• Clippy Test
• Test Postgres (12)
• Test Postgres (13)
• Test Postgres (14)