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

GH-573 fix calc #575

Merged
merged 7 commits into from
Jan 15, 2025
Merged

GH-573 fix calc #575

merged 7 commits into from
Jan 15, 2025

Conversation

ate47
Copy link
Collaborator

@ate47 ate47 commented Jan 14, 2025

Issue resolved (if any): #573

Description of this pull request:

@hmottestad I'm not really sure if it fixes the issue. I replaced everything to long, but the most important part is updateEstimatedLocationArrayBucketSize.

I think the estimatedLocationBucketSize value was too small for the highest value. I kept the nearest power of 2 alignment, but I don't see where it was used.


Please check all the lines before posting the pull request:

  • I've created tests for all my changes
  • My pull request isn't fixing or changing multiple unlinked elements (please create one pull request for each element)
  • I've applied the code formatter (mvn formatter:format on the backend, npm run format on the frontend) before posting my pull request, mvn formatter:validate to validate the formatting on the backend, npm run validate on the frontend
  • All my commits have relevant names
  • I've squashed my commits (if necessary)

@hmottestad
Copy link
Contributor

This looks very good!

I've created a more specific test for this issue:

package com.the_qa_company.qendpoint.core.util.disk;

import org.junit.Assert;
import org.junit.Test;

import java.io.IOException;

public class AbstractLongArrayTest {

	@Test
	public void updateEstimatedLocationArrayBucketSize() {
		AbstractLongArray longArray = new AbstractLongArray() {
			@Override
			public long get(long index) {
				return 0;
			}

			@Override
			public long length() {
				return 0;
			}

			@Override
			public int sizeOf() {
				return 0;
			}

			@Override
			public void resize(long newSize) throws IOException {

			}

			@Override
			public void clear() {

			}

			@Override
			protected void innerSet(long index, long value) {

			}
		};

		for (long i = 0; i < (AbstractLongArray.ESTIMATED_LOCATION_ARRAY_SIZE * 1024L) + 3; i++) {
			testMaxValue(longArray, i);
		}

		for (long i = 1; i > 0 && i < (Long.MAX_VALUE - 1); i *= 2) {
			testMaxValue(longArray, i);
		}

		testMaxValue(longArray, Long.MAX_VALUE);

	}

	private static void testMaxValue(AbstractLongArray longArray, long value) {
		longArray.maxValue = value;
		longArray.updateEstimatedLocationArrayBucketSize();

		long estimatedLocationArrayBucketSize = longArray.getEstimatedLocationArrayBucketSize();

		Assert.assertTrue(estimatedLocationArrayBucketSize > 0);

		longArray.getEstimatedLocation(value, 0, 1);
		longArray.getEstimatedLocationLowerBound(value);
		longArray.getEstimatedLocationUpperBound(value);
	}
}

Screenshot 2025-01-14 at 13 42 41

@hmottestad
Copy link
Contributor

hmottestad commented Jan 14, 2025

It might not need to be aligned to the nearest power of two. I guess I just did that to make the buckets seem more evenly spaced, but it might just be as simple as this.estimatedLocationBucketSize = maxValue/ESTIMATED_LOCATION_ARRAY_SIZE+1;

I can also see that I've been adding 1 to all the indexes. Like this int index = (int) (val / getEstimatedLocationArrayBucketSize() + 1); which I'm now wondering if is actually correct.

If I change my test a bit I'm actually getting another index out of bounds because of that + 1.

package com.the_qa_company.qendpoint.core.util.disk;

import org.junit.Assert;
import org.junit.Test;

import java.io.IOException;
import java.util.Random;

public class AbstractLongArrayTest {

	@Test
	public void updateEstimatedLocationArrayBucketSize() {

		Random random = new Random();
		AbstractLongArray longArray = new AbstractLongArray() {
			@Override
			public long get(long index) {
				return Long.MAX_VALUE-index;
			}

			@Override
			public long length() {
				return AbstractLongArray.ESTIMATED_LOCATION_ARRAY_SIZE * 1024L* 16L;
			}

			@Override
			public int sizeOf() {
				return 0;
			}

			@Override
			public void resize(long newSize) throws IOException {

			}

			@Override
			public void clear() {

			}

			@Override
			protected void innerSet(long index, long value) {

			}
		};

		for (long i = 0; i < (AbstractLongArray.ESTIMATED_LOCATION_ARRAY_SIZE * 1024L) + 3; i++) {
			testMaxValue(longArray, i);
		}

		for (long i = 1; i > 0 && i < (Long.MAX_VALUE - 1); i *= 2) {
			testMaxValue(longArray, i);
		}

		testMaxValue(longArray, Long.MAX_VALUE);
		System.out.println();

		longArray.recalculateEstimatedValueLocation();

		long estimatedLocation = longArray.getEstimatedLocation(Long.MAX_VALUE, -1 , Long.MAX_VALUE);
		long estimatedLocationLowerBound = longArray.getEstimatedLocationLowerBound(Long.MAX_VALUE);
		long estimatedLocationUpperBound = longArray.getEstimatedLocationUpperBound(Long.MAX_VALUE);
		System.out.println(estimatedLocation);
		System.out.println(estimatedLocationLowerBound);
		System.out.println(estimatedLocationUpperBound);
	}

	private static void testMaxValue(AbstractLongArray longArray, long value) {
//		if(value % (1024) == 0) System.out.println(value);

		longArray.maxValue = value;

		longArray.updateEstimatedLocationArrayBucketSize();
//		longArray.recalculateEstimatedValueLocation();

		long estimatedLocationArrayBucketSize = longArray.getEstimatedLocationArrayBucketSize();

		Assert.assertTrue(estimatedLocationArrayBucketSize+"", estimatedLocationArrayBucketSize > 0 );

		longArray.getEstimatedLocation(value, -1, Long.MAX_VALUE);
		longArray.getEstimatedLocationLowerBound(value);
		longArray.getEstimatedLocationUpperBound(value);
	}
}

@ate47 ate47 merged commit 28f373b into dev Jan 15, 2025
20 checks passed
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.

2 participants