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.

Sign up for GitHub

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

feat: a simple LRUCache in frontend #20842

Merged
merged 4 commits into from
Jul 26, 2022
Merged

feat: a simple LRUCache in frontend #20842

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
66a2019
feat: a simple LRUCache in frontend
zhaoyongjie Jul 25, 2022
847724e
addressing comments
zhaoyongjie Jul 25, 2022
28a1ea8
addressing comments
zhaoyongjie Jul 26, 2022
02fbbdd
type
zhaoyongjie Jul 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions superset-frontend/packages/superset-ui-core/src/utils/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export { default as makeSingleton } from './makeSingleton';
export { default as promiseTimeout } from './promiseTimeout';
export { default as logging } from './logging';
export { default as removeDuplicates } from './removeDuplicates';
export { lruCache } from './lruCache';
export * from './featureFlags';
export * from './random';
export * from './typedMemo';
70 changes: 70 additions & 0 deletions superset-frontend/packages/superset-ui-core/src/utils/lruCache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
/**
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

class LRUCache<T> {
private cache: Map<string, T>;

readonly capacity: number;

constructor(capacity: number) {
if (capacity < 1) {
throw new Error('The capacity in LRU must be greater than 0.');
}
this.capacity = capacity;
this.cache = new Map<string, T>();
}

public has(key: string): boolean {
return this.cache.has(key);
}

public get(key: string): T | undefined {
if (this.cache.has(key)) {
const tmp = this.cache.get(key) as T;
this.cache.delete(key);
this.cache.set(key, tmp);
return tmp;
}

zhaoyongjie marked this conversation as resolved.
Show resolved Hide resolved
return undefined;
}

public set(key: string, value: T) {
// Prevent runtime errors
if (typeof key !== 'string') {
throw new TypeError('The LRUCache key must be string.');
}
if (this.cache.size >= this.capacity) {
this.cache.delete(this.cache.keys().next().value);
}
this.cache.set(key, value);
}

public clear() {
this.cache.clear();
}

public get size() {
return this.cache.size;
}
}

export function lruCache<T>(capacity = 100) {
return new LRUCache<T>(capacity);
}
59 changes: 59 additions & 0 deletions superset-frontend/packages/superset-ui-core/test/utils/lruCache.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { lruCache } from '@superset-ui/core';

describe('simple LRUCache', () => {
zhaoyongjie marked this conversation as resolved.
Show resolved Hide resolved
test('initial LRU', () => {
expect(lruCache().capacity).toBe(100);
expect(lruCache(10).capacity).toBe(10);
expect(lruCache(10).size).toBe(0);
expect(() => lruCache(0)).toThrow(Error);
});

test('LRU', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Can we break this into multiple tests? We could have one to test eviction, one to test invalid key types, and one to test cache clearing.

Copy link
Member Author

@zhaoyongjie zhaoyongjie Jul 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the review. This test case is for entire LRU operations. the L43 depends on the cache instance. The purpose of this test case is to prevent incorrect keys from being inserted at runtime, rather than compile time. the clear() method also depends on the cache instance.

Copy link
Member

Choose a reason for hiding this comment

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

Here's an example:

const newCache = () => {
  const cache = lruCache<string>(3);
  cache.set('1', 'a');
  cache.set('2', 'b');
  cache.set('3', 'c');
  return cache;
};

test('initial LRU', () => {
  expect(lruCache().capacity).toBe(100);
  expect(lruCache(10).capacity).toBe(10);
  expect(lruCache(10).size).toBe(0);
  expect(() => lruCache(0)).toThrow(Error);
});

test('correctly evicts LRU value', () => {
  const cache = newCache();
  expect(cache.size).toBe(3);
  cache.set('4', 'd');
  expect(cache.has('1')).toBeFalsy();
  expect(cache.get('1')).toBeUndefined();
});

test('correctly updates LRU value', () => {
  const cache = newCache();
  cache.get('1');
  cache.set('5', 'e');
  expect(cache.has('1')).toBeTruthy();
  expect(cache.has('2')).toBeFalsy();
});

test('throws exception when key is invalid', () => {
  const cache = lruCache<string>(3);
  // @ts-ignore
  expect(() => cache.set(0)).toThrow(TypeError);
});

test('clears the cache', () => {
  const cache = newCache();
  cache.clear();
  expect(cache.size).toBe(0);
  expect(cache.capacity).toBe(3);
});

This way, when a test fails, it's easier to identify what parts of the code are broken. It also avoids state dependencies between different test cases. You can reuse an initial state or optimize for each test.

Copy link
Member Author

@zhaoyongjie zhaoyongjie Jul 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is just a test case, do we really need "modularity"? the original test case has 19 lines and the suggestion from you has 32 lines(removed empty line between test case).

image

the newCache is also not a setup fixture, we can't use it in initial LRU and LRU handle null and undefined. This is a completely additional abstraction.

Copy link
Member

@michael-s-molina michael-s-molina Jul 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think readability and separation of concerns are more important than the number of lines. It's not a single test case because they are different requirements. You could have a bug related to value eviction but have the requirements for clearing the cache and type checking the keys still working.

Copy link
Member Author

Choose a reason for hiding this comment

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

a modularity test case and all-in-one test case are not equivalent, because the each get or set method will affect the entire LRU.
For example:

test('LRU operation', () => {
  const cache = lruCache<string>(3);
  cache.set('1', 'a');
  cache.set('2', 'b');
  cache.set('3', 'c');
  cache.set('4', 'd');
  expect(cache.size).toBe(3);
....

not equal to

const newCache = () => {
  const cache = lruCache<string>(3);
  cache.set('1', 'a');
  cache.set('2', 'b');
  cache.set('3', 'c');
  return cache;
};
test('correctly evicts LRU value', () => {
  const cache = newCache();
  expect(cache.size).toBe(3);
  cache.set('4', 'd');
....

Copy link
Member

Choose a reason for hiding this comment

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

If you move the expect after the set then they will be.

Copy link
Member

Choose a reason for hiding this comment

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

The code that I sent you is just an example. Feel free to improve it. For me, the important part is that the each requirement is in its own test case.

Copy link
Member Author

@zhaoyongjie zhaoyongjie Jul 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@michael-s-molina Sorry, my point was that each call to get and set has an effect on the entire LRU, for example, the correctly evicts LRU value and the correctly updates LRU value should operate on a same LRU instance rather than separately. For this argument, we should probably get someone else to take a look at it as well, and I'd be happy to revise it, thank you very much for the review.

Copy link
Member

Choose a reason for hiding this comment

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

I see no problem in merging these two tests together since they are highly related 😉

const cache = lruCache<string>(3);
cache.set('1', 'a');
cache.set('2', 'b');
cache.set('3', 'c');
cache.set('4', 'd');
expect(cache.size).toBe(3);
expect(cache.has('1')).toBeFalsy();
expect(cache.get('1')).toBeUndefined();
cache.get('2');
cache.set('5', 'e');
expect(cache.has('2')).toBeTruthy();
expect(cache.has('3')).toBeFalsy();
// @ts-ignore
expect(() => cache.set(0)).toThrow(TypeError);
cache.clear();
expect(cache.size).toBe(0);
expect(cache.capacity).toBe(3);
});

test('null and undefined', () => {
const cache = lruCache();
cache.set('a', null);
cache.set('b', undefined);
expect(cache.has('a')).toBeTruthy();
expect(cache.has('b')).toBeTruthy();
expect(cache.get('a')).toBeNull();
expect(cache.get('b')).toBeUndefined();
});
});